Skip to content

feat: complete phase 8 runtime schema freeze#306

Open
flyingrobots wants to merge 22 commits intomainfrom
feat/adr-0008-0009-phase-8
Open

feat: complete phase 8 runtime schema freeze#306
flyingrobots wants to merge 22 commits intomainfrom
feat/adr-0008-0009-phase-8

Conversation

@flyingrobots
Copy link
Owner

@flyingrobots flyingrobots commented Mar 23, 2026

Summary

  • freeze the ADR-0008 runtime schema locally under schemas/runtime/ with validation and conformance docs
  • introduce echo-runtime-schema as the shared home for the frozen logical counters and core runtime ids/key
  • keep Wesley generation explicitly deferred while upstream is unstable, and close out Phase 8 on that honest boundary

Verification

  • cargo test -p echo-runtime-schema --message-format short
  • cargo test -p echo-wasm-abi --message-format short
  • cargo test -p warp-core --lib --message-format short
  • cargo test -p warp-wasm --features engine --lib --message-format short
  • cargo fmt --all --check
  • pnpm schema:runtime:check
  • npx markdownlint-cli2 docs/plans/adr-0008-and-0009.md docs/plans/phase-8-schema-freeze-inventory.md docs/plans/phase-8-runtime-schema-conformance.md docs/plans/phase-8-runtime-schema-mapping-contract.md schemas/runtime/README.md

Summary by CodeRabbit

  • New Features

    • Added a shared runtime-schema crate and GraphQL runtime fragments introducing typed opaque IDs and logical counters.
  • Refactor

    • Replaced raw-byte identifier usages with typed opaque IDs/newtypes across ABI, core, and adapter flows; centralized counter types.
  • Documentation

    • Added Phase 8 schema-freeze plans, conformance audit, mapping contract, fragment README, and ABI/spec/guide updates.
  • Tests

    • Added a runtime-schema validator, CI/local test hooks, and expanded ABI/CBOR and round‑trip tests.
  • Chores

    • Tracked editor settings, updated ignore rules, npm scripts, and dev-tooling entries.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 23, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a new crate crates/echo-runtime-schema providing typed 32‑byte opaque IDs, logical counter newtypes, and composite keys; migrates many local ID/counter definitions to re-export/use that crate across warp-core and echo-wasm-abi; adds GraphQL runtime fragments, a validator, scripts/tests, workspace/editor settings, and related docs.

Changes

Cohort / File(s) Summary
Workspace & Editor config
\.gitignore, .vscode/settings.json, Cargo.toml, package.json
Ignore target-ra/; track !.vscode/settings.json; add workspace member crates/echo-runtime-schema; add schema:runtime:check script and prettier/graphql devDeps.
New crate: echo-runtime-schema
crates/echo-runtime-schema/Cargo.toml, crates/echo-runtime-schema/README.md, crates/echo-runtime-schema/src/lib.rs
New crate exposing RuntimeIdBytes, opaque WorldlineId/HeadId, logical counters (WorldlineTick, GlobalTick, RunId), WriterHeadKey, feature gating (std/serde), serde impls, and tests.
echo-wasm-abi
crates/echo-wasm-abi/Cargo.toml, crates/echo-wasm-abi/src/kernel_port.rs, crates/echo-wasm-abi/src/lib.rs, crates/echo-wasm-abi/src/codec.rs
Use workspace echo-runtime-schema (feature wiring); replace raw Vec<u8>/u64 DTOs with typed IDs/newtypes; add CBOR-focused tests; small allocation-to-owned-string change.
warp-core refactors
crates/warp-core/Cargo.toml, crates/warp-core/src/clock.rs, crates/warp-core/src/head.rs, crates/warp-core/src/worldline.rs, crates/warp-core/src/observation.rs
Remove local ID/counter definitions; pub use types from echo_runtime_schema; update constructors to from_bytes and consumers to as_bytes; remove local WriterHeadKey.
warp-wasm kernel changes
crates/warp-wasm/src/lib.rs, crates/warp-wasm/src/warp_kernel.rs
ABI types switched to typed IDs; replaced fallible parsing helpers with infallible converters; tests updated to use ABI typed IDs.
GraphQL runtime schema fragments
schemas/runtime/*, schemas/runtime/README.md
Add Phase‑8 fragments (identifiers, routing/admission, playback control, scheduler results) and README describing validation workflow and intended ownership.
Schema validation tooling & tests
scripts/validate-runtime-schema-fragments.mjs, scripts/verify-local.sh, tests/hooks/test_runtime_schema_validation.sh
Add Node ESM validator (Prettier + graphql.parse, duplicate/missing-ref checks); integrate into verify script conditional runs; add shell test harness exercising success/failure cases.
Dependency / DAG tooling & tests
docs/dependency-dags.md, scripts/check-append-only.js, scripts/generate-dependency-dags.js, scripts/generate-tasks-dag.js, tests/hooks/test_dependency_dags.sh
Switch TASKS-DAG default to docs/archive/tasks/TASKS-DAG.md; update scripts and DOT metadata; add generator integration test.
Widespread tests & consumers updates
crates/*, crates/ttd-browser/*, crates/echo-dind-harness/*
Update many tests/consumers to construct WorldlineId::from_bytes(...) and use as_bytes() where raw bytes required; adjust MBUS headers and golden vectors accordingly.
Docs & planning
docs/plans/*, docs/spec/SPEC-0009-wasm-abi-v3.md, CHANGELOG.md, CONTRIBUTING.md, docs/guide/cargo-features.md
Add Phase‑8 audit/mapping/conformance docs; update SPEC to document typed IDs (serialize as bytes(32)); add workspace/editor guidance and changelog notes.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant WasmABI as "echo-wasm-abi (ABI)"
  participant WarpCore as "warp-core"
  participant Schema as "echo-runtime-schema"

  Client->>WasmABI: send ControlIntentV1 { AbiWriterHeadKey(AbiWorldlineId, AbiHeadId) }
  WasmABI->>Schema: AbiWorldlineId::from_bytes(...) → WorldlineId
  WasmABI->>Schema: AbiHeadId::from_bytes(...) → HeadId
  WasmABI->>WarpCore: deliver ControlIntent { WriterHeadKey(WorldlineId, HeadId) }
  WarpCore->>Schema: reference GlobalTick / WorldlineTick / RunId (pub use)
  WarpCore-->>Client: acknowledge / process result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

tooling

Poem

Opaque bytes now have a home and name,
Counters march in types, no longer plain.
Fragments linted, validators run,
from_bytes and as_bytes — the refactor is done. 🚀

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: complete phase 8 runtime schema freeze' directly and accurately summarizes the main change: freezing the ADR-0008 runtime schema by introducing a shared crate (echo-runtime-schema), validating GraphQL fragments, and deferring Wesley generation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/adr-0008-0009-phase-8

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2af8725a51

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.gitignore:
- Line 24: Remove the .gitignore exception for .vscode/settings.json or replace
the hardcoded Unix path in that settings file (/tmp/echo-rust-analyzer-target)
with a cross-platform solution: either use a relative path (e.g.,
target/rust-analyzer) or reference an environment-variable fallback (e.g.,
${RA_TARGET_DIR}) so Windows users aren’t broken; then update CONTRIBUTING.md to
document the chosen approach and any editor setup steps and expected optional vs
committed settings so contributors know whether to commit their .vscode settings
or keep local overrides.

In @.vscode/settings.json:
- Around line 2-4: Replace the absolute /tmp path in the rust-analyzer config so
it is repository-relative and portable: update the
"rust-analyzer.cargo.extraEnv" entry to set CARGO_TARGET_DIR to a repo-local
directory name (e.g. "target-ra") instead of "/tmp/echo-rust-analyzer-target" to
avoid platform, collision and permission issues; also add the chosen directory
(e.g. target-ra/) to .gitignore near the existing target-* entries so build
artifacts are not committed.

In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 75-89: WorldlineId exposes as_bytes() but lacks a symmetric
constructor like HeadId::from_bytes, forcing callers to construct it via the
public tuple field; add a const constructor WorldlineId::from_bytes(bytes:
RuntimeIdBytes) -> Self to provide a stable API surface matching HeadId and make
construction explicit and ergonomic alongside the existing as_bytes() method.

In `@crates/echo-wasm-abi/Cargo.toml`:
- Line 21: The Cargo.toml for echo-wasm-abi is forwarding the crate feature
"std" to serde, ciborium and half but not to echo-runtime-schema, so when
building with --features std the schema crate still compiles no-std; fix this by
updating the echo-runtime-schema dependency entry in Cargo.toml to enable its
std feature (mirror how serde/ciborium/half are forwarded) — e.g. change the
echo-runtime-schema declaration to include features = ["std"] (and workspace =
true as already present) so the schema types compile with std when the crate's
std feature is enabled.

In `@schemas/runtime/artifact-b-routing-and-admission.graphql`:
- Around line 111-114: The schema's InboxPolicyBudgeted.maxPerTick field is
documented to be positive but uses plain Int, so enforce the positivity either
by introducing/using a PositiveInt custom scalar in the schema and replacing
maxPerTick: Int! with maxPerTick: PositiveInt! (and add its implementation), or
add explicit runtime validation in the resolver/input handling for the
InboxPolicyBudgeted type that throws a GraphQLError when maxPerTick <= 0;
reference the type name InboxPolicyBudgeted and the field name maxPerTick when
making the change so callers/validators are updated accordingly.

In `@scripts/generate-dependency-dags.js`:
- Around line 562-565: The fallback never runs because parseArgs sets
args.tasksDagPath to the string "TASKS-DAG.md"; update the parseArgs option for
--tasks-dag (the default value for tasksDagPath) to undefined (or remove the
default) so args.tasksDagPath is nullish when the flag is not provided, and keep
the existing tasksDagPath resolution using: const tasksDagPath =
path.resolve(process.cwd(), args.tasksDagPath ??
path.join("docs","archive","tasks","TASKS-DAG.md")); this ensures the fallback
to docs/archive/tasks/TASKS-DAG.md is used.

In `@scripts/verify-local.sh`:
- Around line 710-717: The loop variable name md_file is misleading because the
loop iterates CHANGED_FILES looking for .graphql/.mjs files; rename md_file to a
clearer identifier like changed_file or file_in_changes inside the while loop
and update all references (the case pattern matching and any uses that set
should_validate_runtime_schema) so the logic remains identical but the variable
name accurately reflects it (look for occurrences of CHANGED_FILES, md_file, and
should_validate_runtime_schema to update).
- Around line 732-735: The script calls "node
scripts/validate-runtime-schema-fragments.mjs" when the variable
should_validate_runtime_schema is set but does not verify Node is installed;
update the if-block that checks should_validate_runtime_schema to first verify
Node is available (e.g., use a command existence check against "node"), and if
missing print a clear error and exit non‑zero before attempting to run
validate-runtime-schema-fragments.mjs; reference the existing
should_validate_runtime_schema check and the node invocation so you modify that
branch only.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2cd2f967-fe13-426c-abc7-972cd5c1af2d

📥 Commits

Reviewing files that changed from the base of the PR and between 12b4cab and 2af8725.

⛔ Files ignored due to path filters (3)
  • Cargo.lock is excluded by !**/*.lock
  • docs/assets/dags/tasks-dag.dot is excluded by !**/*.dot
  • docs/assets/dags/tasks-dag.svg is excluded by !**/*.svg
📒 Files selected for processing (34)
  • .gitignore
  • .vscode/settings.json
  • Cargo.toml
  • crates/echo-runtime-schema/Cargo.toml
  • crates/echo-runtime-schema/README.md
  • crates/echo-runtime-schema/src/lib.rs
  • crates/echo-wasm-abi/Cargo.toml
  • crates/echo-wasm-abi/src/kernel_port.rs
  • crates/echo-wasm-abi/src/lib.rs
  • crates/warp-core/Cargo.toml
  • crates/warp-core/src/clock.rs
  • crates/warp-core/src/head.rs
  • crates/warp-core/src/observation.rs
  • crates/warp-core/src/worldline.rs
  • crates/warp-wasm/src/lib.rs
  • crates/warp-wasm/src/warp_kernel.rs
  • docs/dependency-dags.md
  • docs/plans/adr-0008-and-0009.md
  • docs/plans/phase-8-runtime-schema-conformance.md
  • docs/plans/phase-8-runtime-schema-mapping-contract.md
  • docs/plans/phase-8-schema-freeze-inventory.md
  • docs/spec/SPEC-0009-wasm-abi-v3.md
  • package.json
  • schemas/runtime/README.md
  • schemas/runtime/artifact-a-identifiers.graphql
  • schemas/runtime/artifact-b-routing-and-admission.graphql
  • schemas/runtime/artifact-c-playback-control.graphql
  • schemas/runtime/artifact-d-scheduler-results.graphql
  • scripts/check-append-only.js
  • scripts/generate-dependency-dags.js
  • scripts/generate-tasks-dag.js
  • scripts/validate-runtime-schema-fragments.mjs
  • scripts/verify-local.sh
  • tests/hooks/test_runtime_schema_validation.sh

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 76-89: Make WorldlineId's inner field private (remove pub from the
tuple field) and add a constructor method
WorldlineId::from_bytes(RuntimeIdBytes) -> Self (preferably const fn) that
constructs the new opaque id; keep the existing as_bytes(&self) returning
&RuntimeIdBytes. This mirrors HeadId's opacity and provides a canonical factory
to replace direct tuple construction (update tests and call sites from
WorldlineId([...]) to WorldlineId::from_bytes([...])). Ensure all references use
the new constructor and that RuntimeIdBytes remains the parameter/return type.

In `@scripts/generate-dependency-dags.js`:
- Around line 173-175: The fallback date label uses local-time formatting via
formatDateYYYYMMDD(new Date()), causing non-deterministic labels across
timezones; change this to produce a UTC-based date string instead—either update
formatDateYYYYMMDD to use getUTCFullYear/getUTCMonth/getUTCDate or call a new
UTC-aware helper (e.g., formatDateYYYYMMDDUTC) from the fallback assignment that
uses new Date() but formats with UTC methods; ensure the code referencing
generatedAt and the fallback const still returns { mode: "date", label:
<UTC-formatted-string> } so snapshot labels are consistent across environments.

In `@tests/hooks/test_dependency_dags.sh`:
- Line 92: The test's grep is brittle because it looks for the exact attribute
ordering 'i1 -> i2 \[color="red"' in tests/hooks/test_dependency_dags.sh; change
the check that uses that literal grep to a more flexible pattern that matches an
edge line for "i1 -> i2" with a bracketed attribute list that contains
color="red" anywhere (e.g., use a regex like i1 -> i2 \[[^]]*color="red"[^]]*\]
or equivalent POSIX/egrep form) so the assertion no longer depends on attribute
ordering while still verifying the color attribute is present.
- Around line 88-91: Before invoking node on scripts/generate-dependency-dags.js
in the temp workspace ($tmpdir), create a package.json that sets the module type
so Node treats files as ESM; add a step to write a package.json with
{"type":"module"} into $tmpdir (i.e., create the file in the same block that cds
into "$tmpdir" before running node scripts/generate-dependency-dags.js
>"$output_file" 2>&1) so the import-based script runs under ESM semantics.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 685201e4-296b-43db-a931-e4069b76a073

📥 Commits

Reviewing files that changed from the base of the PR and between 2af8725 and 0dc6f36.

📒 Files selected for processing (8)
  • crates/echo-runtime-schema/Cargo.toml
  • crates/echo-runtime-schema/README.md
  • crates/echo-runtime-schema/src/lib.rs
  • crates/echo-wasm-abi/Cargo.toml
  • crates/warp-core/Cargo.toml
  • docs/guide/cargo-features.md
  • scripts/generate-dependency-dags.js
  • tests/hooks/test_dependency_dags.sh

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@CONTRIBUTING.md`:
- Line 106: Replace the machine-specific absolute path
"/Users/james/git/echo/.vscode/settings.json" in the CONTRIBUTING.md sentence
with a repo-relative link to the tracked config, e.g. use
[.vscode/settings.json] (or ./ .vscode/settings.json) so the link works for all
users and does not expose a local filesystem path.

In `@crates/ttd-browser/src/lib.rs`:
- Around line 1206-1210: Update the wasm32 test accessors to match the new
WorldlineId API: replace any use of tuple-field access like wl.0 with the new
method call *wl.as_bytes(). Specifically, change occurrences in
wasm_tests::test_parse_helpers (and related wasm32 tests) that read wl.0 to use
*wl.as_bytes(), and adjust test_parse_worldline_id_inner_valid expectations to
remain consistent with that API.

In `@crates/warp-core/src/provenance_store.rs`:
- Line 2564: Multiple fork tests repeat the literal [99u8; 32] when constructing
WorldlineId via WorldlineId::from_bytes([99u8; 32]); introduce a single test
fixture (either a const like TEST_WORLDLINE_BYTES: [u8; 32] = [99u8; 32] or a
helper fn like test_worldline_id() -> WorldlineId that calls
WorldlineId::from_bytes(TEST_WORLDLINE_BYTES)) and replace each occurrence of
WorldlineId::from_bytes([99u8; 32]) (seen in the file where
WorldlineId::from_bytes is used around the fork tests) with the shared
constant/helper to avoid duplication and reduce drift risk.

In `@schemas/runtime/artifact-a-identifiers.graphql`:
- Around line 49-55: The PositiveInt scalar currently only guarantees "greater
than zero" but must be documented to map to Rust's u32 range; update the
mapping/docs for PositiveInt to explicitly state it represents the full u32
range (1–4,294,967,295) and note that cycle_limit (Option<u32>) and fields
cycleLimit / maxPerTick consume this scalar, so generators that treat GraphQL
Int as 32-bit signed need the explicit u32 upper-bound to accept values above
2,147,483,647; reference PositiveInt, cycle_limit, cycleLimit, maxPerTick and
Option<u32> in the doc update.

In `@schemas/runtime/artifact-b-routing-and-admission.graphql`:
- Around line 67-69: Unify the field name for the exact-head coordinate across
the schema by renaming ExactHeadTarget.key to match IngressTargetInput.headKey
(or vice versa) before the schema freeze; update the GraphQL type definitions
(e.g., ExactHeadTarget and IngressTargetInput) so both use the identical field
name (headKey or key), adjust any associated input/output types and usages in
the same file (including the occurrences around lines 84-89), and ensure all
references, resolvers, and generated DTO/mapper expectations are updated
accordingly so the schema is consistent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: eb4deab0-eee9-46a7-a5e6-0b13108d7c51

📥 Commits

Reviewing files that changed from the base of the PR and between 0dc6f36 and 21a683e.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (36)
  • .gitignore
  • .vscode/settings.json
  • CHANGELOG.md
  • CONTRIBUTING.md
  • crates/echo-dind-harness/tests/digest_golden_vectors.rs
  • crates/echo-runtime-schema/src/lib.rs
  • crates/echo-wasm-abi/Cargo.toml
  • crates/echo-wasm-abi/src/codec.rs
  • crates/ttd-browser/src/lib.rs
  • crates/warp-core/src/coordinator.rs
  • crates/warp-core/src/engine_impl.rs
  • crates/warp-core/src/head.rs
  • crates/warp-core/src/head_inbox.rs
  • crates/warp-core/src/observation.rs
  • crates/warp-core/src/playback.rs
  • crates/warp-core/src/provenance_store.rs
  • crates/warp-core/src/snapshot.rs
  • crates/warp-core/src/worldline.rs
  • crates/warp-core/src/worldline_registry.rs
  • crates/warp-core/src/worldline_state.rs
  • crates/warp-core/tests/checkpoint_fork_tests.rs
  • crates/warp-core/tests/common/mod.rs
  • crates/warp-core/tests/golden_vectors_phase0.rs
  • crates/warp-core/tests/inbox.rs
  • crates/warp-core/tests/invariant_property_tests.rs
  • crates/warp-core/tests/outputs_playback_tests.rs
  • crates/warp-core/tests/slice_theorem_proof.rs
  • crates/warp-wasm/src/warp_kernel.rs
  • docs/guide/cargo-features.md
  • package.json
  • schemas/runtime/artifact-a-identifiers.graphql
  • schemas/runtime/artifact-b-routing-and-admission.graphql
  • schemas/runtime/artifact-d-scheduler-results.graphql
  • scripts/generate-dependency-dags.js
  • scripts/verify-local.sh
  • tests/hooks/test_dependency_dags.sh

@flyingrobots
Copy link
Owner Author

@coderabbitai review please 🥕

@flyingrobots
Copy link
Owner Author

@coderabbitai review please 🥕

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 24, 2026

@flyingrobots: On it. Kicking off a full review now.


🧠 Learnings used
Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-01-24T23:22:15.388Z
Learning: The user (flyingrobots) prefers extremely detailed, pedantic code reviews with no mercy — "GOD-TIER NIGHTMARE CODE REVIEWER" style. They want critical scrutiny, exhaustive analysis, identification of landmines, design questions, and actionable demands on every review.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T20:33:08.471Z
Learning: In flyingrobots/echo `crates/warp-core/src/worldline_state.rs`, `committed_ingress` is an explicit Phase 3 process-lifetime dedupe ledger. Bounding or persisting it would change duplicate-suppression semantics and the storage model. The contract is documented inline in `worldline_state.rs`. Do not flag unbounded growth of `committed_ingress` as a bug in Phase 3 reviews; it is deferred follow-up design work.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T20:33:08.471Z
Learning: In flyingrobots/echo `crates/warp-wasm/src/warp_kernel.rs`, `WarpKernel` is single-writer in the Phase 3 WASM ABI: the kernel only exposes the default worldline and default writer head, so one call to `SchedulerCoordinator::super_tick()` produces at most one committed `StepRecord`. This assumption is documented with an inline note in `warp_kernel.rs`. Do not flag the lack of a `budget` parameter or multi-writer scheduling as a gap in Phase 3 reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/head.rs`, `HeadId` is opaque (no public inner field); `heads_for_worldline()` uses BTreeMap range bounds internally via the opaque key. `WriterHead` derives pause state from `PlaybackMode`; `unpause(PlaybackMode::Paused)` fails deterministically in all build configurations (not just debug). Do not flag these as design gaps in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/head_inbox.rs`, `HeadInbox::ingest()` enforces the canonical content hash (`ingress_id`) in **release builds** (not just `debug_assert`). `IngressEnvelope` fields are private; the hash invariant is upheld at the boundary in all build configurations. Do not flag this as insufficient in future reviews.

Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: docs/archive/AGENTS.md:0-0
Timestamp: 2026-03-14T07:42:53.371Z
Learning: Applies to docs/archive/crates/{warp-core,echo-wasm-abi,echo-scene-port}/**/*.rs : Determinism-critical crates (warp-core, echo-wasm-abi, echo-scene-port) are compiled with RUSTFLAGS="-Dwarnings"; unused imports, dead code, or silenced lints are treated as build failures
✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 24, 2026

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== echo-runtime-schema/src/lib.rs ==="
cat crates/echo-runtime-schema/src/lib.rs

echo ""
echo "=== echo-runtime-schema/Cargo.toml ==="
cat crates/echo-runtime-schema/Cargo.toml

echo ""
echo "=== echo-wasm-abi/src/kernel_port.rs ==="
cat crates/echo-wasm-abi/src/kernel_port.rs

echo ""
echo "=== echo-wasm-abi/src/lib.rs (tests) ==="
cat crates/echo-wasm-abi/src/lib.rs

echo ""
echo "=== scripts/validate-runtime-schema-fragments.mjs ==="
cat scripts/validate-runtime-schema-fragments.mjs

echo ""
echo "=== schemas/runtime/artifact-a-identifiers.graphql ==="
cat schemas/runtime/artifact-a-identifiers.graphql

echo ""
echo "=== schemas/runtime/artifact-d-scheduler-results.graphql ==="
cat schemas/runtime/artifact-d-scheduler-results.graphql

echo ""
echo "=== crates/warp-core/src/head.rs ==="
cat crates/warp-core/src/head.rs

echo ""
echo "=== crates/warp-core/src/worldline.rs ==="
cat crates/warp-core/src/worldline.rs

echo ""
echo "=== crates/warp-core/src/clock.rs ==="
cat crates/warp-core/src/clock.rs

echo ""
echo "=== crates/warp-wasm/src/warp_kernel.rs ==="
cat crates/warp-wasm/src/warp_kernel.rs

echo ""
echo "=== schemas/runtime/README.md ==="
cat schemas/runtime/README.md

echo ""
echo "=== schemas/runtime/artifact-b-routing-and-admission.graphql ==="
cat schemas/runtime/artifact-b-routing-and-admission.graphql

echo ""
echo "=== schemas/runtime/artifact-c-playback-control.graphql ==="
cat schemas/runtime/artifact-c-playback-control.graphql

echo ""
echo "=== crates/echo-wasm-abi/src/codec.rs ==="
cat crates/echo-wasm-abi/src/codec.rs

echo ""
echo "=== crates/warp-core/src/observation.rs (full) ==="
cat crates/warp-core/src/observation.rs

echo ""
echo "=== Cargo.toml (workspace) ==="
cat Cargo.toml

echo ""
echo "=== package.json ==="
cat package.json

echo ""
echo "=== tests/hooks/test_runtime_schema_validation.sh ==="
cat tests/hooks/test_runtime_schema_validation.sh

echo ""
echo "=== scripts/verify-local.sh ==="
cat scripts/verify-local.sh

Length of output: 260485


@flyingrobots

Executive summary

  • Phase 8 runtime schema freeze is correctly reflected across Rust, ABI, and GraphQL fragments. Opaque ids and logical counters are centralized in echo-runtime-schema and correctly re-exported/consumed by warp-core and adapters.
  • ABI v3 wire compatibility is preserved: WorldlineId/HeadId remain bytes(32) on the wire; tests prove CBOR shape and length enforcement.
  • Scheduler-status surface is coherent and documented; warp-wasm maps it consistently.

Blocking

  • None found. The changes build conceptually clean and preserve determinism and wire shape.

High-priority follow-ups (pre-merge optional, post-merge acceptable)

  1. Make the schema validator resilient when npm/npx is absent
  • Today, scripts/validate-runtime-schema-fragments.mjs unconditionally shells out to npx prettier. On many minimal Node installs (Node without npm), npx is missing. verify-local.sh ensures node, but not npx.
  • Fix: probe a small command matrix, using the first that exists: npx prettier; pnpm exec prettier; repo-local node_modules/.bin/prettier. Minimal change shown below.
diff --git a/scripts/validate-runtime-schema-fragments.mjs b/scripts/validate-runtime-schema-fragments.mjs
@@
-import { spawnSync } from "node:child_process";
+import { spawnSync } from "node:child_process";
+import { existsSync } from "node:fs";
@@
 function runPrettierCheck(files) {
   const formattingErrors = [];
 
+  // Try a few execution strategies in order of preference
+  const localPrettier = resolve(REPO_ROOT, "node_modules", ".bin", process.platform === "win32" ? "prettier.cmd" : "prettier");
+  const strategies = [
+    { cmd: "npx", args: ["prettier"] },
+    { cmd: "pnpm", args: ["exec", "prettier"] },
+    { cmd: localPrettier, args: [], onlyIfExists: true },
+  ];
+
+  const pickPrettier = () => {
+    for (const s of strategies) {
+      if (s.onlyIfExists && !existsSync(s.cmd)) continue;
+      const probe = spawnSync(s.cmd, [...s.args, "--version"], { encoding: "utf8" });
+      if (probe.error || probe.status !== 0) continue;
+      return s;
+    }
+    return null;
+  };
+
+  const runner = pickPrettier();
+  if (!runner) {
+    throw new Error("failed to find a runnable prettier (tried npx, pnpm exec, and local node_modules/.bin/prettier)");
+  }
+
   for (const file of files) {
     const source = readFileSync(file, "utf8");
     const syntheticFilePath = join(
       REPO_ROOT,
       "schemas/runtime",
       basename(file),
     );
-    const result = spawnSync(
-      "npx",
-      [
-        "prettier",
-        "--parser",
-        "graphql",
-        "--stdin-filepath",
-        syntheticFilePath,
-      ],
-      {
-        encoding: "utf8",
-        input: source,
-      },
-    );
+    const result = spawnSync(runner.cmd, [...runner.args, "--parser", "graphql", "--stdin-filepath", syntheticFilePath], {
+      encoding: "utf8",
+      input: source,
+    });
@@
     if (result.error) {
-      throw new Error(
-        `failed to run npx prettier for schema validation: ${result.error.message}`,
-      );
+      throw new Error(`failed to run prettier for schema validation: ${result.error.message}`);
     }
  1. Expand negative decode coverage for opaque ids (ABI)
  • kernel_port.rs’s visitor already rejects non-32-byte payloads and arrays longer than 32, but tests only cover 31 bytes via CBOR bytes. Add sequence and 33-byte rejections to prevent regressions in alternate decoders.
  • Minimal additions in crates/echo-wasm-abi/src/lib.rs tests:
#[test]
fn test_worldline_id_rejects_33_byte_sequence() {
    use crate::kernel_port::WorldlineId;
    use ciborium::value::Value;
    #[derive(Debug, PartialEq, Eq, Deserialize)]
    struct Wrapper { id: WorldlineId }
    // Sequence form: [u8; 33]
    let arr = Value::Array((0..33).map(|_| Value::Integer(0u8.into())).collect());
    let bytes = encode_value(&Value::Map(vec![(Value::Text("id".into()), arr)])).unwrap();
    let err = decode_cbor::<Wrapper>(&bytes).unwrap_err();
    assert!(err.toString().contains("32 bytes"));
}

#[test]
fn test_worldline_id_accepts_array_sequence_exactly_32() {
    use crate::kernel_port::WorldlineId;
    use ciborium::value::Value;
    #[derive(Debug, PartialEq, Eq, Deserialize)]
    struct Wrapper { id: WorldlineId }
    let arr = Value::Array((0..32).map(|_| Value::Integer(7u8.into())).collect());
    let bytes = encode_value(&Value::Map(vec![(Value::Text("id".into()), arr)])).unwrap();
    let decoded: Wrapper = decode_cbor(&bytes).unwrap();
    assert_eq!(decoded.id.as_bytes(), &[7u8; 32]);
}
  1. no_std hygiene check (heads-up)
  • echo-wasm-abi is #![no_std] while crates like codec.rs derive thiserror::Error. That trait pulls std::error::Error unless gated. This has worked historically in this repo when building with the std feature enabled in tests; if you intend to keep “pure” no_std builds (no std feature) green on CI, add a quick smoke lane for --no-default-features or gate the derive with cfg(feature = "std"). If CI already covers this, ignore.

Design and correctness review (OK/notes by area)

  • echo-runtime-schema/src/lib.rs

    • Logical counters (WorldlineTick, GlobalTick, RunId) provide const ZERO/MAX, from_raw/as_u64, and checked_* APIs only. This is the right choice to avoid unchecked overflow in Add/Sub impls; Display impl is deterministic. Good.
    • WorldlineId/HeadId are repr(transparent) over [u8; 32], serde transparent when enabled. HeadId::MIN/MAX exist for BTreeMap range bounds; warp-core leverages these in heads_for_worldline(). Good.
    • WriterHeadKey is a simple product type with serde derives; ordering, hashing and equality are well-defined. Good.
  • warp-core

    • head.rs re-exports shared id/key types; make_head_id(label) domain-separates with "head:" and blake3. Tests document domain separation and collision boundaries against other ident domains. Good.
    • heads_for_worldline() uses [MIN, MAX] sentinels for BTreeMap range scan; iteration is O(log n + k). Good.
    • worldline.rs/observation.rs now round-trip through the ABI wrappers cleanly via as_bytes()/from_bytes(). Observation artifacts’ hash input encodes the fully-typed ABI DTO; artifact hashing remains domain-separated and deterministic. Good.
  • echo-wasm-abi/src/kernel_port.rs

    • Opaque id wrappers WorldlineId/HeadId serialize to bytes; deserialize_opaque_id accepts bytes and arrays with strict 32-byte enforcement. The Visitor error messages communicate expected length clearly. Good.
    • ABI DTOs align with the GraphQL fragments: HeadEligibility, SchedulerMode/State, WorkState, RunCompletion, WriterHeadKey, Observation* and SchedulerStatus surfaces match “artifact-d” and keep snake_case tags.
    • Tests cover: control-intent canonical encoding (hex fixture), worldline id CBOR bytes shape, 31-B rejection, control-intent head-key round-trip. Consider adding the two negative cases above.
  • warp-wasm/src/warp_kernel.rs

    • parse_worldline_id/parse_head_id are infallible since ABI types are already typed 32-byte ids — correct after the adapter surface change.
    • Scheduler state machine:
      • Start rejects Some(0) cycle_limit deterministically; invariant preservation is documented.
      • run_id allocation uses checked_increment; overflow would surface an engine error (pre-scheduler-state mutation) — correct ordering.
      • WorkState is derived from per-head inbox emptiness and admission/pause state; latest_cycle_global_tick is recorded even if no commits happened; latest_commit_global_tick updates when a non-empty record set returns. This respects “<=” invariant.
      • Stop while Inactive is a no-op preserving last_run_completion, matching doc comments. Good.
  • GraphQL runtime fragments (schemas/runtime/*.graphql)

    • Artifact A-D establish: opaque ids/counters; routing/admission; playback control; scheduler-mode/state/result. The PositiveInt scalar is explicitly documented where needed (Budgeted and UntilIdle).
    • Validation scope (prettier + cross-fragment references) is deliberately narrow, which is fine for a local freeze. See hardening item (1) for portability.
  • Tooling and hooks

    • tests/hooks/test_runtime_schema_validation.sh exercises both success and missing-type failure; greps for the reference name — good signal quality.
    • scripts/verify-local.sh wires runtime schema validation under docs linting; if node is absent it errors out with a clear message — good.

Non-blocking nits and small polish

  • echo-runtime-schema: Consider impl From<$ty> for u64 and TryFrom for $ty behind a feature if you want ergonomic conversions without encouraging unchecked arithmetic. Keeping only checked_* is the safer default; not a request, just a note.
  • echo-wasm-abi: You may want a tiny property test asserting that old bytes(32) payloads for worldline_id/head_id from a v2 encoding decode into the new typed wrappers unchanged (defensive future-proofing). The new tests already show the shape; property test would make drift harder.

What I verified explicitly

  • WorldlineId/HeadId wire encoding remains bytes(32) and fails on wrong lengths (CBOR bytes) — covered by tests.
  • Head range scans in warp-core rely on HeadId::MIN/MAX from the shared crate — used consistently.
  • RunId/GlobalTick/WorldlineTick moved cleanly to echo-runtime-schema; warp-core:clock now only re-exports — no duplicate definitions.

I retained prior context from earlier reviews:

  • I did not flag “unbounded committed_ingress” or multi-writer scheduling; both are known Phase 3 assumptions in this repo and intentionally out-of-scope for this phase.

If you want, I can open follow-up issues for: (a) schema-validator portability (npx fallback), (b) ABI id negative decode cases, and (c) optional no_std smoke lane for echo-wasm-abi.


🧠 Learnings used
Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-01-24T23:22:15.388Z
Learning: The user (flyingrobots) prefers extremely detailed, pedantic code reviews with no mercy — "GOD-TIER NIGHTMARE CODE REVIEWER" style. They want critical scrutiny, exhaustive analysis, identification of landmines, design questions, and actionable demands on every review.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T20:33:08.471Z
Learning: In flyingrobots/echo `crates/warp-core/src/worldline_state.rs`, `committed_ingress` is an explicit Phase 3 process-lifetime dedupe ledger. Bounding or persisting it would change duplicate-suppression semantics and the storage model. The contract is documented inline in `worldline_state.rs`. Do not flag unbounded growth of `committed_ingress` as a bug in Phase 3 reviews; it is deferred follow-up design work.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T20:33:08.471Z
Learning: In flyingrobots/echo `crates/warp-wasm/src/warp_kernel.rs`, `WarpKernel` is single-writer in the Phase 3 WASM ABI: the kernel only exposes the default worldline and default writer head, so one call to `SchedulerCoordinator::super_tick()` produces at most one committed `StepRecord`. This assumption is documented with an inline note in `warp_kernel.rs`. Do not flag the lack of a `budget` parameter or multi-writer scheduling as a gap in Phase 3 reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/head.rs`, `HeadId` is opaque (no public inner field); `heads_for_worldline()` uses BTreeMap range bounds internally via the opaque key. `WriterHead` derives pause state from `PlaybackMode`; `unpause(PlaybackMode::Paused)` fails deterministically in all build configurations (not just debug). Do not flag these as design gaps in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/head_inbox.rs`, `HeadInbox::ingest()` enforces the canonical content hash (`ingress_id`) in **release builds** (not just `debug_assert`). `IngressEnvelope` fields are private; the hash invariant is upheld at the boundary in all build configurations. Do not flag this as insufficient in future reviews.

Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: docs/archive/AGENTS.md:0-0
Timestamp: 2026-03-14T07:42:53.371Z
Learning: Applies to docs/archive/crates/{warp-core,echo-wasm-abi,echo-scene-port}/**/*.rs : Determinism-critical crates (warp-core, echo-wasm-abi, echo-scene-port) are compiled with RUSTFLAGS="-Dwarnings"; unused imports, dead code, or silenced lints are treated as build failures
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

♻️ Duplicate comments (4)
crates/ttd-browser/src/lib.rs (1)

1206-1209: ⚠️ Potential issue | 🟠 Major

The wasm32 test module still uses the old private-field accessor.

This native test was migrated, but wasm_tests::test_parse_helpers at Line 1110 still does wl.0. WorldlineId is opaque now, so wasm32 test builds remain broken until that assertion also switches to *wl.as_bytes().

🔧 Required follow-up
-        assert_eq!(wl.0, [0u8; 32]);
+        assert_eq!(*wl.as_bytes(), [0u8; 32]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/ttd-browser/src/lib.rs` around lines 1206 - 1209, Update the wasm32
test in wasm_tests::test_parse_helpers to stop using the old private-field
accessor on WorldlineId (wl.0) and use the public accessor instead: call
parse_worldline_id_inner (or reuse the existing test variable wl) and assert
against *wl.as_bytes() similarly to the native test; replace any occurrence of
wl.0 with *wl.as_bytes() so the opaque WorldlineId is accessed via its
as_bytes() method.
docs/plans/phase-8-runtime-schema-conformance.md (1)

85-87: ⚠️ Potential issue | 🟠 Major

u32 does not map cleanly to GraphQL Int!.

GraphQL Int tops out at 2,147,483,647, so max_per_tick: u32 cannot be described as a clean Int! mapping across its full runtime range. The audit should either call out the signed-32-bit ceiling explicitly or constrain the allowed range instead of blessing the mapping as-is.

What range does the GraphQL specification allow for the built-in `Int` scalar?

As per coding guidelines, "Echo is a deterministic simulation engine. Documentation accuracy matters — especially anything touching determinism guarantees, hash stability, or canonical ordering. Flag factual errors and stale cross-references."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/plans/phase-8-runtime-schema-conformance.md` around lines 85 - 87, The
doc incorrectly states that InboxPolicy::Budgeted { max_per_tick: u32 } maps
cleanly to GraphQL maxPerTick: Int! — GraphQL Int is a signed 32-bit value with
range -2,147,483,648 to 2,147,483,647, so update the text to either (a)
explicitly call out the 2,147,483,647 positive ceiling and note the semantic
validation required for u32 values above that, or (b) change the schema to use a
constrained/custom scalar or validation rule that limits maxPerTick to <=
2,147,483,647 (and document that constraint) so the mapping is accurate;
reference InboxPolicy::Budgeted, max_per_tick and maxPerTick in the update.
crates/echo-runtime-schema/src/lib.rs (1)

19-27: 🛠️ Refactor suggestion | 🟠 Major

Hide the raw u64 field on the shared counters.

pub struct $name(pub u64) makes the representation itself part of the cross-crate API, bypasses the checked helper surface, and leaves the public field undocumented. Before this crate becomes the canonical shared owner, make the field private and force external code onto from_raw(...) / as_u64().

As per coding guidelines, "Every public API across crates (warp-core, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/echo-runtime-schema/src/lib.rs` around lines 19 - 27, The public tuple
field on counters created by the logical_counter! macro exposes the u64
representation and bypasses helpers; change the macro so the generated pub
struct $name has a private field (e.g., struct $name(u64)) and expose a
documented public API: a checked constructor from_raw(...) and an accessor
as_u64() (or similar) and ensure both functions have rustdoc comments describing
invariants/usage; update any existing impls or derives inside logical_counter!
to use the private field and refer to these helpers so external crates must use
from_raw/as_u64 rather than accessing the raw u64 directly.
schemas/runtime/artifact-b-routing-and-admission.graphql (1)

67-69: ⚠️ Potential issue | 🟠 Major

Normalize the exact-head field name before freezing this schema.

Line 68 uses key, while Line 88 uses headKey for the same exact-head coordinate. Freezing both names bakes an avoidable asymmetry into every DTO, mapper, and doc that touches this surface. Pick one name on both input and output types.

Patch
 type ExactHeadTarget {
-    key: WriterHeadKey!
+    headKey: WriterHeadKey!
 }

Also applies to: 84-89

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@schemas/runtime/artifact-b-routing-and-admission.graphql` around lines 67 -
69, The ExactHeadTarget GraphQL type uses inconsistent field names for the same
coordinate (field "key" on ExactHeadTarget vs "headKey" elsewhere); pick one
canonical name (e.g., headKey) and update all schema definitions and related
input/output types so the field name is identical across ExactHeadTarget and
every related type (including the other occurrence at lines 84–89 referenced in
the review), then regenerate DTOs/mappers/docs so the symbol ExactHeadTarget and
any usages reference the chosen field name uniformly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/echo-runtime-schema/README.md`:
- Around line 18-19: Update the README sentence about serde feature gating to
reflect the crate's default behavior: state that serde derives are feature-gated
and consumers must enable the crate's `serde` feature unless they are using the
crate with default features enabled (or unless they compile with
`default-features = false` to force explicit opt-in); reference the `serde`
feature name and the `default-features = false` flag so readers know when
explicit enabling is required.

In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 79-80: WorldlineId and HeadId newtypes currently use
#[serde(transparent)] which serializes the inner [u8;32] as a 32-element integer
array instead of a byte string; implement explicit serde::Serialize and
serde::Deserialize for WorldlineId and HeadId to serialize/deserialze as
canonical bytes(32) (e.g., as a 32-byte byte string for CBOR/JSON) and update
WriterHeadKey usage to rely on those impls; add a unit test that serializes and
deserializes WorldlineId and HeadId (and a WriterHeadKey instance) to verify
round-trip produces the spec-compliant bytes(32) representation.

In `@crates/echo-wasm-abi/src/lib.rs`:
- Around line 519-535: The test test_worldline_id_rejects_non_32_byte_payloads
only checks a 31-byte underflow; add a symmetric overlong case to ensure
deserialization rejects >32 bytes too by constructing a CBOR map with "id" set
to 33 bytes and asserting decode_cbor::<Wrapper>(&bytes).unwrap_err() contains
"32 bytes"; modify the test (which uses WorldlineId and Wrapper and calls
decode_cbor) to include both the 31-byte and 33-byte failure assertions to pin
strict 32-byte enforcement from both sides.

In `@crates/warp-wasm/src/warp_kernel.rs`:
- Around line 170-176: The helpers parse_worldline_id and parse_head_id are not
parsers and should be renamed to to_core_worldline_id and to_core_head_id (or
similar) to reflect direct conversion; change their signatures to return
WorldlineId and HeadId directly (no Result) by returning
WorldlineId::from_bytes(*worldline_id.as_bytes()) and
HeadId::from_bytes(*head_id.as_bytes()); likewise rename and change
parse_head_key (and the similar function referenced around lines 460-464) to
to_core_head_key and return the HeadKey directly instead of Result, and update
all call sites (including the call at the former line 439) to remove dead error
handling/unwraps accordingly.

In `@docs/dependency-dags.md`:
- Line 89: The docs line referencing the scheduled workflow uses incorrect
platform casing; update the sentence that mentions
`.github/workflows/refresh-dependency-dags.yml` so the platform name reads
"GitHub" instead of "GITHUB" (search for the Generator line mentioning
`scripts/generate-tasks-dag.js` and
`.github/workflows/refresh-dependency-dags.yml` in docs/dependency-dags.md and
replace the uppercase token with the proper "GitHub" casing).

In `@docs/plans/phase-8-runtime-schema-conformance.md`:
- Around line 127-133: Update the blocker paragraph to reflect the current
typed-id migration: remove or reword the claim that `HeadId`, `WorldlineId`, and
`WriterHeadKey` are still exposed as raw `Vec<u8>` and instead list only the
actual runtime schema types that remain as raw byte vectors (if any); reference
the typed ABI wrappers for `HeadId`, `WorldlineId`, and `WriterHeadKey` so the
note is narrowed to the true remaining raw-byte ids and mark the previous
statement as stale/updated to avoid misleading the audit.

In `@docs/plans/phase-8-schema-freeze-inventory.md`:
- Around line 97-105: Artifact A's identifier list omits WorldlineId, causing
inconsistency with the Phase 8 freeze candidates and downstream artifacts;
update the Artifact A identifier enumeration (the block listing HeadId,
WriterHeadKey, IntentKind, WorldlineTick, GlobalTick) to include WorldlineId so
the artifact definition matches the freeze candidate set and downstream
dependencies that reference WorldlineId.

In `@scripts/validate-runtime-schema-fragments.mjs`:
- Around line 70-88: The script currently hard-codes spawning "npx" (see the
spawnSync call that runs prettier) which breaks pnpm-only environments; change
the launcher resolution to try a cascade: first "npx", then "pnpm exec", and
finally fall back to the local node_modules/.bin prettier executable if present,
using the same args and input so pnpm users can run the check. Also fix
extractTypeNames (the function that parses GraphQL field type expressions) so it
stops parsing when it encounters directive markers (@) and ignores text inside
string literals (respecting quoted boundaries), ensuring identifiers inside
directives or strings aren't collected as type names; update the
tokenization/state machine in extractTypeNames to exit on '@' and to treat
quoted strings as atomic tokens.
- Around line 112-147: The current sanitizeLines and extractTypeNames
implementations are brittle string-based heuristics; replace them by parsing the
SDL with a GraphQL parser (e.g., graphql-js parse/visit) and derive the
information from the AST: use parse(source, { noLocation: false }) to get
DocumentNode, iterate DocumentNode.definitions to collect type/operation
definitions (replacing sanitizeLines' role) and use a visitor to collect
NamedType nodes (replacing extractTypeNames) while ignoring descriptions,
comments and directive argument content; ensure you import and use graphql's
parse/visit functions, return the same shape expected by downstream code, and
remove the fragile line-based logic in sanitizeLines and extractTypeNames.

In `@tests/hooks/test_dependency_dags.sh`:
- Around line 98-103: The test currently calls fail and then immediately cats
"$tmpdir/docs/assets/dags/issue-deps.dot", which under set -e can hide the
original assertion if the DOT file is missing; change the branch so that after
the grep condition fails you first call fail with a clear message and only
attempt to dump the DOT file conditionally (e.g. check for existence or make the
cat a non-failing operation) to avoid a secondary “file not found” error; target
the block that references "$tmpdir/docs/assets/dags/issue-deps.dot" and the grep
check (the if grep -Eq ... then ... else ... fi) and ensure the fail() call runs
before any optional diagnostic dump or that the dump is guarded so it cannot
cause the test to exit with a different error.

---

Duplicate comments:
In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 19-27: The public tuple field on counters created by the
logical_counter! macro exposes the u64 representation and bypasses helpers;
change the macro so the generated pub struct $name has a private field (e.g.,
struct $name(u64)) and expose a documented public API: a checked constructor
from_raw(...) and an accessor as_u64() (or similar) and ensure both functions
have rustdoc comments describing invariants/usage; update any existing impls or
derives inside logical_counter! to use the private field and refer to these
helpers so external crates must use from_raw/as_u64 rather than accessing the
raw u64 directly.

In `@crates/ttd-browser/src/lib.rs`:
- Around line 1206-1209: Update the wasm32 test in
wasm_tests::test_parse_helpers to stop using the old private-field accessor on
WorldlineId (wl.0) and use the public accessor instead: call
parse_worldline_id_inner (or reuse the existing test variable wl) and assert
against *wl.as_bytes() similarly to the native test; replace any occurrence of
wl.0 with *wl.as_bytes() so the opaque WorldlineId is accessed via its
as_bytes() method.

In `@docs/plans/phase-8-runtime-schema-conformance.md`:
- Around line 85-87: The doc incorrectly states that InboxPolicy::Budgeted {
max_per_tick: u32 } maps cleanly to GraphQL maxPerTick: Int! — GraphQL Int is a
signed 32-bit value with range -2,147,483,648 to 2,147,483,647, so update the
text to either (a) explicitly call out the 2,147,483,647 positive ceiling and
note the semantic validation required for u32 values above that, or (b) change
the schema to use a constrained/custom scalar or validation rule that limits
maxPerTick to <= 2,147,483,647 (and document that constraint) so the mapping is
accurate; reference InboxPolicy::Budgeted, max_per_tick and maxPerTick in the
update.

In `@schemas/runtime/artifact-b-routing-and-admission.graphql`:
- Around line 67-69: The ExactHeadTarget GraphQL type uses inconsistent field
names for the same coordinate (field "key" on ExactHeadTarget vs "headKey"
elsewhere); pick one canonical name (e.g., headKey) and update all schema
definitions and related input/output types so the field name is identical across
ExactHeadTarget and every related type (including the other occurrence at lines
84–89 referenced in the review), then regenerate DTOs/mappers/docs so the symbol
ExactHeadTarget and any usages reference the chosen field name uniformly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 33ede45b-a8aa-4f5f-b399-91a409595a16

📥 Commits

Reviewing files that changed from the base of the PR and between 12b4cab and 67f57c7.

⛔ Files ignored due to path filters (4)
  • Cargo.lock is excluded by !**/*.lock
  • docs/assets/dags/tasks-dag.dot is excluded by !**/*.dot
  • docs/assets/dags/tasks-dag.svg is excluded by !**/*.svg
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (58)
  • .gitignore
  • .vscode/settings.json
  • CHANGELOG.md
  • CONTRIBUTING.md
  • Cargo.toml
  • crates/echo-dind-harness/tests/digest_golden_vectors.rs
  • crates/echo-runtime-schema/Cargo.toml
  • crates/echo-runtime-schema/README.md
  • crates/echo-runtime-schema/src/lib.rs
  • crates/echo-wasm-abi/Cargo.toml
  • crates/echo-wasm-abi/src/codec.rs
  • crates/echo-wasm-abi/src/kernel_port.rs
  • crates/echo-wasm-abi/src/lib.rs
  • crates/ttd-browser/src/lib.rs
  • crates/warp-core/Cargo.toml
  • crates/warp-core/src/clock.rs
  • crates/warp-core/src/coordinator.rs
  • crates/warp-core/src/engine_impl.rs
  • crates/warp-core/src/head.rs
  • crates/warp-core/src/head_inbox.rs
  • crates/warp-core/src/observation.rs
  • crates/warp-core/src/playback.rs
  • crates/warp-core/src/provenance_store.rs
  • crates/warp-core/src/snapshot.rs
  • crates/warp-core/src/worldline.rs
  • crates/warp-core/src/worldline_registry.rs
  • crates/warp-core/src/worldline_state.rs
  • crates/warp-core/tests/checkpoint_fork_tests.rs
  • crates/warp-core/tests/common/mod.rs
  • crates/warp-core/tests/golden_vectors_phase0.rs
  • crates/warp-core/tests/inbox.rs
  • crates/warp-core/tests/invariant_property_tests.rs
  • crates/warp-core/tests/outputs_playback_tests.rs
  • crates/warp-core/tests/slice_theorem_proof.rs
  • crates/warp-wasm/src/lib.rs
  • crates/warp-wasm/src/warp_kernel.rs
  • docs/ROADMAP/backlog/README.md
  • docs/ROADMAP/backlog/tooling-misc.md
  • docs/dependency-dags.md
  • docs/guide/cargo-features.md
  • docs/plans/adr-0008-and-0009.md
  • docs/plans/phase-8-runtime-schema-conformance.md
  • docs/plans/phase-8-runtime-schema-mapping-contract.md
  • docs/plans/phase-8-schema-freeze-inventory.md
  • docs/spec/SPEC-0009-wasm-abi-v3.md
  • package.json
  • schemas/runtime/README.md
  • schemas/runtime/artifact-a-identifiers.graphql
  • schemas/runtime/artifact-b-routing-and-admission.graphql
  • schemas/runtime/artifact-c-playback-control.graphql
  • schemas/runtime/artifact-d-scheduler-results.graphql
  • scripts/check-append-only.js
  • scripts/generate-dependency-dags.js
  • scripts/generate-tasks-dag.js
  • scripts/validate-runtime-schema-fragments.mjs
  • scripts/verify-local.sh
  • tests/hooks/test_dependency_dags.sh
  • tests/hooks/test_runtime_schema_validation.sh

@flyingrobots
Copy link
Owner Author

@coderabbitai review please 🥕

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 24, 2026

@flyingrobots: Triggering a full re-review now. 🥕


🧠 Learnings used
Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/head.rs`, `HeadId` is opaque (no public inner field); `heads_for_worldline()` uses BTreeMap range bounds internally via the opaque key. `WriterHead` derives pause state from `PlaybackMode`; `unpause(PlaybackMode::Paused)` fails deterministically in all build configurations (not just debug). Do not flag these as design gaps in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/coordinator.rs`, `SchedulerCoordinator::peek_order()` derives runnable order from the head registry directly. Tick counters use checked arithmetic and fail deterministically on overflow. Do not flag these as stale-cache or silent-overflow risks in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/head_inbox.rs`, `HeadInbox::ingest()` enforces the canonical content hash (`ingress_id`) in **release builds** (not just `debug_assert`). `IngressEnvelope` fields are private; the hash invariant is upheld at the boundary in all build configurations. Do not flag this as insufficient in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 300
File: crates/warp-core/src/head.rs:0-0
Timestamp: 2026-03-11T04:29:30.253Z
Learning: In flyingrobots/echo (crates/warp-core and sibling crates), `#[allow(clippy::unwrap_used)]` on `#[cfg(test)]` modules is the established, codebase-wide convention. Test code uses `.unwrap()` extensively for brevity and crash-on-failure semantics. Do not flag `clippy::unwrap_used` suppressions on test modules in this codebase in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-01-24T23:22:15.388Z
Learning: The user (flyingrobots) prefers extremely detailed, pedantic code reviews with no mercy — "GOD-TIER NIGHTMARE CODE REVIEWER" style. They want critical scrutiny, exhaustive analysis, identification of landmines, design questions, and actionable demands on every review.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-19T19:27:08.539Z
Learning: In flyingrobots/echo `crates/warp-core/src/worldline.rs`, `apply_to_worldline_state(...)` (line ~141) is intentionally a pure patch-op applicator over `state.warp_state` only. It does not and must not mutate `tick_history` or `last_snapshot`, because it lacks authoritative provenance receipts/snapshots. That replay metadata is rebuilt exclusively from stored provenance via `hydrate_replay_metadata(...)` at `crates/warp-core/src/provenance_store.rs:716`, invoked by the checkpoint restore path at line ~781. Mutating replay metadata inside `apply_to_worldline_state(...)` would duplicate provenance-derived state and risk diverging from the stored commit/receipt stream. Do not flag this omission as a bug in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 300
File: crates/warp-core/src/coordinator.rs:36-42
Timestamp: 2026-03-11T04:29:43.838Z
Learning: In flyingrobots/echo, `super_tick()` in `crates/warp-core/src/coordinator.rs` intentionally uses `expect()` to panic on invariant violations (runnable head referencing a worldline not in the registry). This is a programmer error, not a recoverable runtime condition. The panic is documented in the `Panics` section (lines ~103-106). Do not flag this as a violation of the no-unwrap/expect rule in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 298
File: crates/warp-core/src/provenance_store.rs:387-398
Timestamp: 2026-03-09T07:57:39.706Z
Learning: In flyingrobots/echo, `LocalProvenanceStore::atom_history()` in `crates/warp-core/src/provenance_store.rs` intentionally uses an O(history_length) backward scan with `out_slots` filtering and early termination at creation. It is a diagnostic "Show Me Why" TTD query (not a per-tick simulation hot path), so the overhead is acceptable. A per-slot reverse index was explicitly deferred to profiling. Do not flag this scan as a performance issue in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T20:33:08.471Z
Learning: In flyingrobots/echo `crates/warp-core/src/worldline_state.rs`, `committed_ingress` is an explicit Phase 3 process-lifetime dedupe ledger. Bounding or persisting it would change duplicate-suppression semantics and the storage model. The contract is documented inline in `worldline_state.rs`. Do not flag unbounded growth of `committed_ingress` as a bug in Phase 3 reviews; it is deferred follow-up design work.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T20:33:08.471Z
Learning: In flyingrobots/echo `crates/warp-wasm/src/warp_kernel.rs`, `WarpKernel` is single-writer in the Phase 3 WASM ABI: the kernel only exposes the default worldline and default writer head, so one call to `SchedulerCoordinator::super_tick()` produces at most one committed `StepRecord`. This assumption is documented with an inline note in `warp_kernel.rs`. Do not flag the lack of a `budget` parameter or multi-writer scheduling as a gap in Phase 3 reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/worldline_registry.rs`, `WorldlineRegistry::register()` intentionally returns `bool` (not `Result`) and is `#[must_use]`. Duplicate registration is surfaced as `RuntimeError::DuplicateWorldline` at the runtime surface layer. This is the finalized design; do not flag the `bool` return as insufficient in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 304
File: crates/warp-wasm/src/warp_kernel.rs:375-428
Timestamp: 2026-03-21T01:45:53.256Z
Learning: In flyingrobots/echo `crates/warp-wasm/src/warp_kernel.rs`, `WarpKernel`'s `ControlIntentV1::Start { mode: UntilIdle { ... } }` runs the entire scheduler loop synchronously inside `apply_control_intent` and clears active run state before returning. Hosts always observe `state = Inactive` plus `last_run_completion` after `dispatch_intent` returns. `ControlIntentV1::Stop` is a no-op once a synchronous run has completed; it is only meaningful for implementations that persist runs across calls. This is the specified contract, documented in `docs/spec/SPEC-0009-wasm-abi-v3.md` lines 191-199 and in `crates/echo-wasm-abi/src/kernel_port.rs` lines 252-256. Do not flag the synchronous loop model or the Stop no-op behavior as bugs in future reviews.
✅ Actions performed

Full review triggered.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

♻️ Duplicate comments (2)
docs/plans/phase-8-runtime-schema-conformance.md (1)

56-59: ⚠️ Potential issue | 🟠 Major

The HeadId row is stale and contradicts the later blocker section.

The table says the ABI still keeps a byte DTO for HeadId, but the later section correctly says the ABI now carries typed wrappers for HeadId/WorldlineId/WriterHeadKey. With crates/echo-wasm-abi/src/lib.rs and crates/warp-wasm/src/warp_kernel.rs already consuming typed ids directly, this row misstates the freeze boundary.

Suggested edit
-| `HeadId`             | `crates/echo-runtime-schema/src/lib.rs` | Aligned             | Opaque hash-backed newtype matches the scalar intent. `warp-core` now consumes the shared owner and the ABI keeps a byte DTO. |
+| `HeadId`             | `crates/echo-runtime-schema/src/lib.rs` | Aligned             | Opaque hash-backed newtype matches the scalar intent. `warp-core` and the ABI now consume the typed shared owner; CBOR still encodes it as bytes(32). |

As per coding guidelines, "Documentation accuracy matters — especially anything touching determinism guarantees, hash stability, or canonical ordering. Flag factual errors and stale cross-references."

Also applies to: 127-132

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/plans/phase-8-runtime-schema-conformance.md` around lines 56 - 59, The
table entry for `HeadId` is stale and contradicts the blocker section; update
the table row(s) so they state that the ABI now carries typed wrappers for
`HeadId`, `WorldlineId`, and `WriterHeadKey` (remove any claim that the ABI
keeps a byte DTO) to match the later text and current code in
`crates/echo-wasm-abi/src/lib.rs` and `crates/warp-wasm/src/warp_kernel.rs`;
ensure both the `HeadId` and `WorldlineId` rows (and the similar rows at
127-132) reflect the "Aligned" status with typed ABI wrappers and adjust the
Notes column to reference consumption by the listed crates.
scripts/validate-runtime-schema-fragments.mjs (1)

75-79: ⚠️ Potential issue | 🟠 Major

Prefer the workspace-pinned Prettier before the indirect launchers.

This validator is part of the runtime-freeze toolchain, so formatter resolution should be deterministic. Probing npx before the repo-local binary keeps version selection tied to external launcher behavior instead of the workspace install. Prefer node_modules/.bin/prettier first, then pnpm exec, and keep npx only as a last-resort --no-install fallback.

Patch
     const candidates = [
-        { command: "npx", prefix: ["prettier"] },
-        { command: "pnpm", prefix: ["exec", "prettier"] },
-        { command: localPrettier, prefix: [], requireExists: true },
+        { command: localPrettier, prefix: [], requireExists: true },
+        { command: "pnpm", prefix: ["exec", "prettier"] },
+        { command: "npx", prefix: ["--no-install", "prettier"] },
     ];

Also applies to: 85-97, 111-119

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/validate-runtime-schema-fragments.mjs` around lines 75 - 79, Change
the Prettier resolution order to prefer the workspace-pinned binary first: put
the entry that uses localPrettier (requireExists: true) at the front of the
candidates array, followed by the pnpm exec entry, and leave the npx entry last
(use npx with --no-install as a safe fallback). Update every place where the
candidates array is defined (the instances around the current candidates
declaration and the other occurrences referenced in the review: the blocks
covering lines ~85-97 and ~111-119) so they all use the same ordering
(localPrettier, pnpm exec, then npx) to ensure deterministic formatter
resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 214-228: Add serde sequence-path regression tests exercising
RuntimeIdVisitor::visit_seq: add one acceptance test that deserializes a
32-element sequence of u8 values into the expected [u8; 32] (round-trip or
direct deserialization asserting Ok) and one rejection test that attempts to
deserialize a 33-element sequence and asserts an Err with invalid_length; target
the same module that defines RuntimeIdVisitor (the code around fn visit_seq and
the RuntimeIdVisitor type) so the sequence-based path is exercised in addition
to the existing CBOR byte-string tests.

In `@crates/echo-wasm-abi/src/lib.rs`:
- Around line 484-545: Add tests mirroring the WorldlineId CBOR checks for
HeadId to prevent drift: create a round-trip CBOR test (e.g.,
test_head_id_round_trip_uses_cbor_bytes) that encodes a small Wrapper struct
containing HeadId::from_bytes([7u8;32]) and asserts the CBOR map stores
Value::Bytes(32) and that decoding yields the same HeadId, and create a
rejection test (e.g., test_head_id_rejects_non_32_byte_payloads) that tries to
decode 31- and 33-byte Value::Bytes payloads into a Wrapper and asserts the
errors mention "32 bytes"; also add equivalent tests for the types that embed
HeadId (WriterHeadKey and ControlIntentV1::SetHeadEligibility) following the
same round-trip and 31/33-byte rejection patterns so all wrappers enforce the
exact-32-byte invariant.

In `@docs/plans/phase-8-schema-freeze-inventory.md`:
- Around line 44-46: The docs omitted the scalar PositiveInt from the frozen
supporting-types list and affected artifacts; add PositiveInt to the list of
supporting types alongside WorldlineId/InboxAddress/etc., include it in the
candidate table of frozen types, and ensure Artifact A (the identifiers
artifact) explicitly declares the PositiveInt scalar so Artifact B (the
routing/admission artifact) that references InboxPolicyBudgeted.maxPerTick and
InboxPolicyInput.maxPerTick is fully specified; update the three affected
sections (supporting-type summary, candidate table, and Artifact A description)
to mention PositiveInt.

In `@package.json`:
- Line 13: The project has duplicated invocation paths for the runtime schema
validator: the npm script "schema:runtime:check" and the shell wrapper in
scripts/verify-local.sh; consolidate by having scripts/verify-local.sh call the
npm script instead of invoking the validator directly (or remove the
package.json script if you prefer the shell wrapper), e.g., update
verify-local.sh to run npm run schema:runtime:check (or yarn run
schema:runtime:check) where it currently shells into
scripts/validate-runtime-schema-fragments.mjs so there is a single canonical
entrypoint named "schema:runtime:check".

In `@schemas/runtime/artifact-a-identifiers.graphql`:
- Around line 17-23: Update the WorldlineId scalar documentation to specify it
is a fixed-width, 32-byte (256-bit) hash-backed opaque identifier (similar to
HeadId): state that implementations MUST produce and accept exactly 32 bytes and
that generators should treat it as a 32-byte hash value; update the semantic
rule paragraph for WorldlineId to explicitly require the 32-byte invariant and
mention any canonical encoding (e.g., raw bytes or hex) expected by the
runtime/ABI so consumers like generators have an unambiguous contract.

---

Duplicate comments:
In `@docs/plans/phase-8-runtime-schema-conformance.md`:
- Around line 56-59: The table entry for `HeadId` is stale and contradicts the
blocker section; update the table row(s) so they state that the ABI now carries
typed wrappers for `HeadId`, `WorldlineId`, and `WriterHeadKey` (remove any
claim that the ABI keeps a byte DTO) to match the later text and current code in
`crates/echo-wasm-abi/src/lib.rs` and `crates/warp-wasm/src/warp_kernel.rs`;
ensure both the `HeadId` and `WorldlineId` rows (and the similar rows at
127-132) reflect the "Aligned" status with typed ABI wrappers and adjust the
Notes column to reference consumption by the listed crates.

In `@scripts/validate-runtime-schema-fragments.mjs`:
- Around line 75-79: Change the Prettier resolution order to prefer the
workspace-pinned binary first: put the entry that uses localPrettier
(requireExists: true) at the front of the candidates array, followed by the pnpm
exec entry, and leave the npx entry last (use npx with --no-install as a safe
fallback). Update every place where the candidates array is defined (the
instances around the current candidates declaration and the other occurrences
referenced in the review: the blocks covering lines ~85-97 and ~111-119) so they
all use the same ordering (localPrettier, pnpm exec, then npx) to ensure
deterministic formatter resolution.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7e8663ec-9f7c-4beb-9fba-01feffefbba7

📥 Commits

Reviewing files that changed from the base of the PR and between 67f57c7 and d560ba2.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (17)
  • CHANGELOG.md
  • CONTRIBUTING.md
  • crates/echo-runtime-schema/Cargo.toml
  • crates/echo-runtime-schema/README.md
  • crates/echo-runtime-schema/src/lib.rs
  • crates/echo-wasm-abi/src/lib.rs
  • crates/warp-core/src/provenance_store.rs
  • crates/warp-wasm/src/warp_kernel.rs
  • docs/dependency-dags.md
  • docs/plans/phase-8-runtime-schema-conformance.md
  • docs/plans/phase-8-schema-freeze-inventory.md
  • package.json
  • schemas/runtime/artifact-a-identifiers.graphql
  • schemas/runtime/artifact-b-routing-and-admission.graphql
  • scripts/validate-runtime-schema-fragments.mjs
  • tests/hooks/test_dependency_dags.sh
  • tests/hooks/test_runtime_schema_validation.sh

Comment on lines +214 to +228
fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
where
A: SeqAccess<'de>,
{
let mut bytes = [0u8; 32];
for (index, byte) in bytes.iter_mut().enumerate() {
*byte = seq
.next_element()?
.ok_or_else(|| de::Error::invalid_length(index, &self))?;
}
if seq.next_element::<u8>()?.is_some() {
return Err(de::Error::invalid_length(33, &self));
}
Ok(bytes)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Add a regression test for the sequence-based serde path.

RuntimeIdVisitor::visit_seq() is the code that enforces exact length for decoders emitting a 32-element u8 array, but the current serde tests only round-trip CBOR byte strings. Add one acceptance case for an exact 32-element sequence and one rejection case for 33 elements so alternate decoders cannot regress silently.

Also applies to: 340-385

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/echo-runtime-schema/src/lib.rs` around lines 214 - 228, Add serde
sequence-path regression tests exercising RuntimeIdVisitor::visit_seq: add one
acceptance test that deserializes a 32-element sequence of u8 values into the
expected [u8; 32] (round-trip or direct deserialization asserting Ok) and one
rejection test that attempts to deserialize a 33-element sequence and asserts an
Err with invalid_length; target the same module that defines RuntimeIdVisitor
(the code around fn visit_seq and the RuntimeIdVisitor type) so the
sequence-based path is exercised in addition to the existing CBOR byte-string
tests.

"docs:build": "vitepress build docs",
"test:e2e": "playwright test"
"test:e2e": "playwright test",
"schema:runtime:check": "node scripts/validate-runtime-schema-fragments.mjs"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Avoid a second source of truth for runtime-schema validation.

package.json now exposes schema:runtime:check, but the real local gate in scripts/verify-local.sh still shells into the validator directly. That leaves two invocation paths to keep in sync. Make the shell gate delegate to this script, or drop the wrapper here.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package.json` at line 13, The project has duplicated invocation paths for the
runtime schema validator: the npm script "schema:runtime:check" and the shell
wrapper in scripts/verify-local.sh; consolidate by having
scripts/verify-local.sh call the npm script instead of invoking the validator
directly (or remove the package.json script if you prefer the shell wrapper),
e.g., update verify-local.sh to run npm run schema:runtime:check (or yarn run
schema:runtime:check) where it currently shells into
scripts/validate-runtime-schema-fragments.mjs so there is a single canonical
entrypoint named "schema:runtime:check".

Comment on lines +17 to +23
"""
Opaque stable identifier for a worldline.

Semantic rule: this remains an opaque identifier even though implementations
often derive it from the initial worldline state.
"""
scalar WorldlineId
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Freeze the byte-width invariant for WorldlineId.

HeadId is documented as fixed-width/hash-backed, but WorldlineId is only described as “opaque”. The runtime and ABI in this PR now enforce exact 32-byte worldline ids, so leaving that out makes Artifact A underspecified for generators.

Suggested edit
 """
 Opaque stable identifier for a worldline.
 
-Semantic rule: this remains an opaque identifier even though implementations
-often derive it from the initial worldline state.
+Semantic rule: this is a fixed-width 32-byte opaque identifier. Consumers must
+preserve opacity and byte width even though implementations may derive it from
+the initial worldline state.
 """
 scalar WorldlineId
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@schemas/runtime/artifact-a-identifiers.graphql` around lines 17 - 23, Update
the WorldlineId scalar documentation to specify it is a fixed-width, 32-byte
(256-bit) hash-backed opaque identifier (similar to HeadId): state that
implementations MUST produce and accept exactly 32 bytes and that generators
should treat it as a 32-byte hash value; update the semantic rule paragraph for
WorldlineId to explicitly require the 32-byte invariant and mention any
canonical encoding (e.g., raw bytes or hex) expected by the runtime/ABI so
consumers like generators have an unambiguous contract.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 24, 2026

@flyingrobots: On it — kicking off another full review. 🥕


🧠 Learnings used
Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/worldline_registry.rs`, `WorldlineRegistry::register()` intentionally returns `bool` (not `Result`) and is `#[must_use]`. Duplicate registration is surfaced as `RuntimeError::DuplicateWorldline` at the runtime surface layer. This is the finalized design; do not flag the `bool` return as insufficient in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/head.rs`, `HeadId` is opaque (no public inner field); `heads_for_worldline()` uses BTreeMap range bounds internally via the opaque key. `WriterHead` derives pause state from `PlaybackMode`; `unpause(PlaybackMode::Paused)` fails deterministically in all build configurations (not just debug). Do not flag these as design gaps in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/coordinator.rs`, `SchedulerCoordinator::peek_order()` derives runnable order from the head registry directly. Tick counters use checked arithmetic and fail deterministically on overflow. Do not flag these as stale-cache or silent-overflow risks in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 300
File: crates/warp-core/src/coordinator.rs:36-42
Timestamp: 2026-03-11T04:29:43.838Z
Learning: In flyingrobots/echo, `super_tick()` in `crates/warp-core/src/coordinator.rs` intentionally uses `expect()` to panic on invariant violations (runnable head referencing a worldline not in the registry). This is a programmer error, not a recoverable runtime condition. The panic is documented in the `Panics` section (lines ~103-106). Do not flag this as a violation of the no-unwrap/expect rule in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T20:33:08.471Z
Learning: In flyingrobots/echo `crates/warp-wasm/src/warp_kernel.rs`, `WarpKernel` is single-writer in the Phase 3 WASM ABI: the kernel only exposes the default worldline and default writer head, so one call to `SchedulerCoordinator::super_tick()` produces at most one committed `StepRecord`. This assumption is documented with an inline note in `warp_kernel.rs`. Do not flag the lack of a `budget` parameter or multi-writer scheduling as a gap in Phase 3 reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T20:33:08.471Z
Learning: In flyingrobots/echo `crates/warp-core/src/worldline_state.rs`, `committed_ingress` is an explicit Phase 3 process-lifetime dedupe ledger. Bounding or persisting it would change duplicate-suppression semantics and the storage model. The contract is documented inline in `worldline_state.rs`. Do not flag unbounded growth of `committed_ingress` as a bug in Phase 3 reviews; it is deferred follow-up design work.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-19T19:27:08.539Z
Learning: In flyingrobots/echo `crates/warp-core/src/worldline.rs`, `apply_to_worldline_state(...)` (line ~141) is intentionally a pure patch-op applicator over `state.warp_state` only. It does not and must not mutate `tick_history` or `last_snapshot`, because it lacks authoritative provenance receipts/snapshots. That replay metadata is rebuilt exclusively from stored provenance via `hydrate_replay_metadata(...)` at `crates/warp-core/src/provenance_store.rs:716`, invoked by the checkpoint restore path at line ~781. Mutating replay metadata inside `apply_to_worldline_state(...)` would duplicate provenance-derived state and risk diverging from the stored commit/receipt stream. Do not flag this omission as a bug in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-03-12T09:30:36.487Z
Learning: In flyingrobots/echo `crates/warp-core/src/head_inbox.rs`, `HeadInbox::ingest()` enforces the canonical content hash (`ingress_id`) in **release builds** (not just `debug_assert`). `IngressEnvelope` fields are private; the hash invariant is upheld at the boundary in all build configurations. Do not flag this as insufficient in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 304
File: crates/warp-wasm/src/warp_kernel.rs:375-428
Timestamp: 2026-03-21T01:45:53.256Z
Learning: In flyingrobots/echo `crates/warp-wasm/src/warp_kernel.rs`, `WarpKernel`'s `ControlIntentV1::Start { mode: UntilIdle { ... } }` runs the entire scheduler loop synchronously inside `apply_control_intent` and clears active run state before returning. Hosts always observe `state = Inactive` plus `last_run_completion` after `dispatch_intent` returns. `ControlIntentV1::Stop` is a no-op once a synchronous run has completed; it is only meaningful for implementations that persist runs across calls. This is the specified contract, documented in `docs/spec/SPEC-0009-wasm-abi-v3.md` lines 191-199 and in `crates/echo-wasm-abi/src/kernel_port.rs` lines 252-256. Do not flag the synchronous loop model or the Stop no-op behavior as bugs in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 300
File: crates/warp-core/src/head.rs:0-0
Timestamp: 2026-03-11T04:29:30.253Z
Learning: In flyingrobots/echo (crates/warp-core and sibling crates), `#[allow(clippy::unwrap_used)]` on `#[cfg(test)]` modules is the established, codebase-wide convention. Test code uses `.unwrap()` extensively for brevity and crash-on-failure semantics. Do not flag `clippy::unwrap_used` suppressions on test modules in this codebase in future reviews.

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 0
File: :0-0
Timestamp: 2026-01-24T23:22:15.388Z
Learning: The user (flyingrobots) prefers extremely detailed, pedantic code reviews with no mercy — "GOD-TIER NIGHTMARE CODE REVIEWER" style. They want critical scrutiny, exhaustive analysis, identification of landmines, design questions, and actionable demands on every review.

Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: docs/archive/AGENTS.md:0-0
Timestamp: 2026-03-14T07:42:53.371Z
Learning: Applies to docs/archive/crates/{warp-core,echo-wasm-abi,echo-scene-port}/**/*.rs : Determinism-critical crates (warp-core, echo-wasm-abi, echo-scene-port) are compiled with RUSTFLAGS="-Dwarnings"; unused imports, dead code, or silenced lints are treated as build failures

Learnt from: flyingrobots
Repo: flyingrobots/echo PR: 298
File: crates/warp-core/src/provenance_store.rs:387-398
Timestamp: 2026-03-09T07:57:39.706Z
Learning: In flyingrobots/echo, `LocalProvenanceStore::atom_history()` in `crates/warp-core/src/provenance_store.rs` intentionally uses an O(history_length) backward scan with `out_slots` filtering and early termination at creation. It is a diagnostic "Show Me Why" TTD query (not a per-tick simulation hot path), so the overhead is acceptable. A per-slot reverse index was explicitly deferred to profiling. Do not flag this scan as a performance issue in future reviews.

Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-07T19:39:32.763Z
Learning: Applies to crates/{warp-core,echo-wasm-abi,echo-scene-port}/**/*.rs : All determinism-critical crates (warp-core, echo-wasm-abi, echo-scene-port) must be compiled with RUSTFLAGS="-Dwarnings"; unused imports, dead code, or silenced lints are treated as build failures
✅ Actions performed

Full review triggered.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (7)
package.json (1)

13-13: 🧹 Nitpick | 🔵 Trivial

Consolidate runtime schema validation to one canonical entrypoint.

Line 13 adds a package script, but scripts/verify-local.sh still calls the validator file directly. Keep a single path (prefer delegating verify-local.sh to pnpm schema:runtime:check) to prevent drift.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package.json` at line 13, Consolidate the runtime schema validation by
changing calls in verify-local.sh to invoke the new package script instead of
the validator file directly: replace any direct invocation of
scripts/validate-runtime-schema-fragments.mjs (e.g., node
scripts/validate-runtime-schema-fragments.mjs) with a call to the package script
"pnpm run schema:runtime:check" so the canonical entrypoint is
schema:runtime:check and the validator path is not duplicated.
crates/echo-wasm-abi/src/lib.rs (1)

441-545: ⚠️ Potential issue | 🟡 Minor

HeadId still appears to lack symmetric exact-32 wire coverage.

SetHeadEligibility proves the nested happy path, but it does not pin HeadId/WriterHeadKey against 31/33-byte payloads the way WorldlineId is now pinned. Add the same direct round-trip and negative cases here so the two opaque 32-byte ID wrappers cannot drift independently.

Verify whether equivalent tests already exist anywhere under crates/echo-wasm-abi; expected result is that only the nested SetHeadEligibility path appears, not a direct HeadId exact-32 rejection test.

#!/bin/bash
set -euo pipefail

sed -n '439,545p' crates/echo-wasm-abi/src/lib.rs
rg -n -C2 --type=rust 'fn test_.*head_id|HeadId::from_bytes|WriterHeadKey|32 bytes' crates/echo-wasm-abi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/echo-wasm-abi/src/lib.rs` around lines 441 - 545, Add direct tests for
HeadId (and WriterHeadKey if desired) mirroring the WorldlineId tests: create a
Wrapper struct containing a HeadId, assert CBOR round-trip serializes as
Value::Bytes(32 bytes) and decodes back to the same HeadId (like
test_worldline_id_round_trip_uses_cbor_bytes), and add negative cases that feed
31- and 33-byte Value::Bytes payloads and assert decoding fails with an error
mentioning "32 bytes" (like test_worldline_id_rejects_non_32_byte_payloads).
Locate the test additions near the existing WorldlineId tests and reference
HeadId::from_bytes, WriterHeadKey (for nested tests), and any existing
SetHeadEligibility assertions to ensure symmetric coverage.
crates/ttd-browser/src/lib.rs (1)

1206-1209: ⚠️ Potential issue | 🟠 Major

Finish the wasm32 WorldlineId test migration.

The native test was updated here, but wasm_tests::test_parse_helpers still reads wl.0 at Line 1110. That leaves the wasm32 test target on the pre-freeze API and will fail as soon as those tests are built.

Patch
     fn test_parse_helpers() {
         let wl = parse_worldline_id(&[0u8; 32]).unwrap();
-        assert_eq!(wl.0, [0u8; 32]);
+        assert_eq!(*wl.as_bytes(), [0u8; 32]);

         let invalid = parse_worldline_id(&[0u8; 16]);
         assert!(invalid.is_err());
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/ttd-browser/src/lib.rs` around lines 1206 - 1209, The wasm32 test
helper wasm_tests::test_parse_helpers still accesses the old tuple field (wl.0)
of WorldlineId; update it to use the current API (call as_bytes() on the
WorldlineId result) consistent with the native test changes: locate
parse_worldline_id_inner and the wasm_tests::test_parse_helpers reference to
wl.0, change the access to use wl.unwrap().as_bytes() (or call as_bytes() on the
unwrapped WorldlineId) so the wasm test uses the new WorldlineId API.
schemas/runtime/artifact-a-identifiers.graphql (1)

8-15: ⚠️ Potential issue | 🟠 Major

Freeze the exact byte width for HeadId and WorldlineId.

Artifact A is supposed to be the frozen boundary contract, but these docs still leave the ABI-critical width implicit. The runtime crate and ABI wrappers now treat both IDs as exact 32-byte values; without that sentence, generators can still model them as variable-length bytes or strings and remain "schema compliant".

Doc patch
 """
 Opaque stable identifier for a writer head.

-Semantic rule: this is a fixed-width hash-backed identifier. Schema consumers
+Semantic rule: this is an exact 32-byte hash-backed identifier. Schema consumers
 must preserve opacity and must not reinterpret it as a human-readable string
 label or derive ordering semantics beyond byte-stable identity.
 """
 scalar HeadId

 """
 Opaque stable identifier for a worldline.

-Semantic rule: this remains an opaque identifier even though implementations
-often derive it from the initial worldline state.
+Semantic rule: this is an exact 32-byte opaque identifier. Consumers must
+preserve opacity and byte width even though implementations often derive it
+from the initial worldline state.
 """
 scalar WorldlineId

Also applies to: 17-23

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@schemas/runtime/artifact-a-identifiers.graphql` around lines 8 - 15, Update
the scalar docs to explicitly freeze the ABI width: change the HeadId and
WorldlineId docblocks to state they are exact 32-byte (256-bit) opaque
identifiers and must be encoded/handled as fixed 32-byte values (not
variable-length strings or byte arrays). Locate the scalar definitions for
HeadId and WorldlineId in this file (symbols HeadId and WorldlineId) and replace
the current vague "fixed-width hash-backed identifier" wording with a precise
sentence such as "This is an opaque, ABI-fixed 32-byte identifier; consumers
must preserve opacity and treat it as exactly 32 bytes." Apply the same precise
wording to both docblocks so generators cannot model them as variable-length.
docs/plans/phase-8-schema-freeze-inventory.md (2)

97-106: ⚠️ Potential issue | 🟡 Minor

Artifact A enumeration is missing PositiveInt.

The scalar is defined in artifact-a-identifiers.graphql alongside the other identifiers and counters but not listed here.

Proposed fix
 ### Artifact A: Runtime identifiers and logical counters
 
 - `HeadId`
 - `WorldlineId`
 - `WriterHeadKey`
 - `IntentKind`
 - `WorldlineTick`
 - `GlobalTick`
+- `PositiveInt`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/plans/phase-8-schema-freeze-inventory.md` around lines 97 - 106, The
docs list for "Artifact A: Runtime identifiers and logical counters" forgot to
include the PositiveInt scalar defined alongside other identifiers in
artifact-a-identifiers.graphql; update the enumeration under Artifact A to
include `PositiveInt` (alongside `HeadId`, `WorldlineId`, `WriterHeadKey`,
`IntentKind`, `WorldlineTick`, `GlobalTick`) and ensure the source reference to
artifact-a-identifiers.graphql explicitly mentions PositiveInt so the schema and
docs remain in sync.

44-46: ⚠️ Potential issue | 🟡 Minor

PositiveInt is missing from the supporting-type list.

The schema fragment artifact-a-identifiers.graphql defines scalar PositiveInt which is referenced by artifact-b-routing-and-admission.graphql for InboxPolicyBudgeted.maxPerTick. This document lists supporting types at line 45-46 but omits PositiveInt.

Proposed fix
 5. Freezing those runtime surfaces also requires a few supporting types that the
    earlier plan text underspecified: `WorldlineId`, `InboxAddress`, `SeekThen`,
-   `SchedulerMode`, and `RunId`.
+   `SchedulerMode`, `RunId`, and `PositiveInt`.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/plans/phase-8-schema-freeze-inventory.md` around lines 44 - 46, The
supporting-type list omitted the PositiveInt scalar used by
InboxPolicyBudgeted.maxPerTick; update the supporting types to include
PositiveInt so the runtime freeze plan accounts for the scalar referenced in
artifact-a-identifiers.graphql (used by
artifact-b-routing-and-admission.graphql) and ensure any mention of
InboxPolicyBudgeted.maxPerTick is covered by the added PositiveInt entry.
crates/echo-runtime-schema/src/lib.rs (1)

214-228: 🧹 Nitpick | 🔵 Trivial

Sequence-path deserialization lacks explicit test coverage.

visit_seq enforces exactly 32 elements (lines 218-227) but no test exercises this path. The CBOR tests use Value::Bytes, not Value::Array. A past review comment flagged this gap.

Add tests that:

  1. Deserialize a 32-element u8 array into WorldlineId/HeadId.
  2. Reject a 33-element array with invalid_length.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/echo-runtime-schema/src/lib.rs` around lines 214 - 228, Add unit tests
that cover the sequence-path in visit_seq by deserializing array forms into
WorldlineId and HeadId: construct a serde_cbor/serde Value::Array (or equivalent
CBOR array bytes) containing exactly 32 u8 elements, call the crate's
Deserialize (e.g., deserialize/from_slice) for WorldlineId and HeadId and assert
success and equality to the expected 32-byte value; then construct a 33-element
u8 array and assert deserialization fails with de::Error::invalid_length (or
results in an Err matching invalid_length). Reference the visit_seq
implementation and exercise WorldlineId and HeadId deserialization to ensure
both the valid-32 and invalid-33 paths are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/ROADMAP/backlog/tooling-misc.md`:
- Around line 397-445: Update the "T-10-8-10: Feature-Gate Contract
Verification" task spec to mandate the maintenance entrypoint be a cargo xtask
command (e.g., "cargo xtask feature-gate-verify") by adding that shape to the
Requirements (R2) and Acceptance Criteria (AC1) so the verification is
implemented under the repo's xtask surface; apply the same change to the other
backlog item in this document so both tasks explicitly require a cargo xtask
entrypoint and mention xtask/ and .cargo/config.toml as the expected
location/configuration.

In `@scripts/generate-tasks-dag.js`:
- Line 10: INPUT_FILE is currently used both for reading the filesystem and for
emitting DOT labels, which causes Windows backslashes to be escaped and produce
non-deterministic output; fix by separating the two concerns: keep a
filesystem-native path (e.g., INPUT_FILE_FS = path.join(...)) for fs operations
and create a POSIX-normalized display path (e.g., INPUT_FILE_DISPLAY =
path.posix.join(...) or use path.posix.normalize) for any DOT label emission
passed into escapeDotString(), and update all places that emit file paths to use
INPUT_FILE_DISPLAY (including the similar uses around the escapeDotString()
calls referenced in the 175-185 area).

In `@scripts/validate-runtime-schema-fragments.mjs`:
- Around line 11-17: DEFINITION_KIND_NAMES currently omits GraphQL interfaces so
nodes with kind "InterfaceTypeDefinition" won't be recognized; update the
DEFINITION_KIND_NAMES Map (the constant named DEFINITION_KIND_NAMES in
scripts/validate-runtime-schema-fragments.mjs) to include
["InterfaceTypeDefinition", "interface"] so interface definitions are collected
like other types and won't trigger false missing-type errors.

---

Duplicate comments:
In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 214-228: Add unit tests that cover the sequence-path in visit_seq
by deserializing array forms into WorldlineId and HeadId: construct a
serde_cbor/serde Value::Array (or equivalent CBOR array bytes) containing
exactly 32 u8 elements, call the crate's Deserialize (e.g.,
deserialize/from_slice) for WorldlineId and HeadId and assert success and
equality to the expected 32-byte value; then construct a 33-element u8 array and
assert deserialization fails with de::Error::invalid_length (or results in an
Err matching invalid_length). Reference the visit_seq implementation and
exercise WorldlineId and HeadId deserialization to ensure both the valid-32 and
invalid-33 paths are covered.

In `@crates/echo-wasm-abi/src/lib.rs`:
- Around line 441-545: Add direct tests for HeadId (and WriterHeadKey if
desired) mirroring the WorldlineId tests: create a Wrapper struct containing a
HeadId, assert CBOR round-trip serializes as Value::Bytes(32 bytes) and decodes
back to the same HeadId (like test_worldline_id_round_trip_uses_cbor_bytes), and
add negative cases that feed 31- and 33-byte Value::Bytes payloads and assert
decoding fails with an error mentioning "32 bytes" (like
test_worldline_id_rejects_non_32_byte_payloads). Locate the test additions near
the existing WorldlineId tests and reference HeadId::from_bytes, WriterHeadKey
(for nested tests), and any existing SetHeadEligibility assertions to ensure
symmetric coverage.

In `@crates/ttd-browser/src/lib.rs`:
- Around line 1206-1209: The wasm32 test helper wasm_tests::test_parse_helpers
still accesses the old tuple field (wl.0) of WorldlineId; update it to use the
current API (call as_bytes() on the WorldlineId result) consistent with the
native test changes: locate parse_worldline_id_inner and the
wasm_tests::test_parse_helpers reference to wl.0, change the access to use
wl.unwrap().as_bytes() (or call as_bytes() on the unwrapped WorldlineId) so the
wasm test uses the new WorldlineId API.

In `@docs/plans/phase-8-schema-freeze-inventory.md`:
- Around line 97-106: The docs list for "Artifact A: Runtime identifiers and
logical counters" forgot to include the PositiveInt scalar defined alongside
other identifiers in artifact-a-identifiers.graphql; update the enumeration
under Artifact A to include `PositiveInt` (alongside `HeadId`, `WorldlineId`,
`WriterHeadKey`, `IntentKind`, `WorldlineTick`, `GlobalTick`) and ensure the
source reference to artifact-a-identifiers.graphql explicitly mentions
PositiveInt so the schema and docs remain in sync.
- Around line 44-46: The supporting-type list omitted the PositiveInt scalar
used by InboxPolicyBudgeted.maxPerTick; update the supporting types to include
PositiveInt so the runtime freeze plan accounts for the scalar referenced in
artifact-a-identifiers.graphql (used by
artifact-b-routing-and-admission.graphql) and ensure any mention of
InboxPolicyBudgeted.maxPerTick is covered by the added PositiveInt entry.

In `@package.json`:
- Line 13: Consolidate the runtime schema validation by changing calls in
verify-local.sh to invoke the new package script instead of the validator file
directly: replace any direct invocation of
scripts/validate-runtime-schema-fragments.mjs (e.g., node
scripts/validate-runtime-schema-fragments.mjs) with a call to the package script
"pnpm run schema:runtime:check" so the canonical entrypoint is
schema:runtime:check and the validator path is not duplicated.

In `@schemas/runtime/artifact-a-identifiers.graphql`:
- Around line 8-15: Update the scalar docs to explicitly freeze the ABI width:
change the HeadId and WorldlineId docblocks to state they are exact 32-byte
(256-bit) opaque identifiers and must be encoded/handled as fixed 32-byte values
(not variable-length strings or byte arrays). Locate the scalar definitions for
HeadId and WorldlineId in this file (symbols HeadId and WorldlineId) and replace
the current vague "fixed-width hash-backed identifier" wording with a precise
sentence such as "This is an opaque, ABI-fixed 32-byte identifier; consumers
must preserve opacity and treat it as exactly 32 bytes." Apply the same precise
wording to both docblocks so generators cannot model them as variable-length.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ac7c8b57-ecbb-409d-9065-0672f405a259

📥 Commits

Reviewing files that changed from the base of the PR and between 12b4cab and d560ba2.

⛔ Files ignored due to path filters (4)
  • Cargo.lock is excluded by !**/*.lock
  • docs/assets/dags/tasks-dag.dot is excluded by !**/*.dot
  • docs/assets/dags/tasks-dag.svg is excluded by !**/*.svg
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (58)
  • .gitignore
  • .vscode/settings.json
  • CHANGELOG.md
  • CONTRIBUTING.md
  • Cargo.toml
  • crates/echo-dind-harness/tests/digest_golden_vectors.rs
  • crates/echo-runtime-schema/Cargo.toml
  • crates/echo-runtime-schema/README.md
  • crates/echo-runtime-schema/src/lib.rs
  • crates/echo-wasm-abi/Cargo.toml
  • crates/echo-wasm-abi/src/codec.rs
  • crates/echo-wasm-abi/src/kernel_port.rs
  • crates/echo-wasm-abi/src/lib.rs
  • crates/ttd-browser/src/lib.rs
  • crates/warp-core/Cargo.toml
  • crates/warp-core/src/clock.rs
  • crates/warp-core/src/coordinator.rs
  • crates/warp-core/src/engine_impl.rs
  • crates/warp-core/src/head.rs
  • crates/warp-core/src/head_inbox.rs
  • crates/warp-core/src/observation.rs
  • crates/warp-core/src/playback.rs
  • crates/warp-core/src/provenance_store.rs
  • crates/warp-core/src/snapshot.rs
  • crates/warp-core/src/worldline.rs
  • crates/warp-core/src/worldline_registry.rs
  • crates/warp-core/src/worldline_state.rs
  • crates/warp-core/tests/checkpoint_fork_tests.rs
  • crates/warp-core/tests/common/mod.rs
  • crates/warp-core/tests/golden_vectors_phase0.rs
  • crates/warp-core/tests/inbox.rs
  • crates/warp-core/tests/invariant_property_tests.rs
  • crates/warp-core/tests/outputs_playback_tests.rs
  • crates/warp-core/tests/slice_theorem_proof.rs
  • crates/warp-wasm/src/lib.rs
  • crates/warp-wasm/src/warp_kernel.rs
  • docs/ROADMAP/backlog/README.md
  • docs/ROADMAP/backlog/tooling-misc.md
  • docs/dependency-dags.md
  • docs/guide/cargo-features.md
  • docs/plans/adr-0008-and-0009.md
  • docs/plans/phase-8-runtime-schema-conformance.md
  • docs/plans/phase-8-runtime-schema-mapping-contract.md
  • docs/plans/phase-8-schema-freeze-inventory.md
  • docs/spec/SPEC-0009-wasm-abi-v3.md
  • package.json
  • schemas/runtime/README.md
  • schemas/runtime/artifact-a-identifiers.graphql
  • schemas/runtime/artifact-b-routing-and-admission.graphql
  • schemas/runtime/artifact-c-playback-control.graphql
  • schemas/runtime/artifact-d-scheduler-results.graphql
  • scripts/check-append-only.js
  • scripts/generate-dependency-dags.js
  • scripts/generate-tasks-dag.js
  • scripts/validate-runtime-schema-fragments.mjs
  • scripts/verify-local.sh
  • tests/hooks/test_dependency_dags.sh
  • tests/hooks/test_runtime_schema_validation.sh

Comment on lines +397 to +445
## T-10-8-10: Feature-Gate Contract Verification

**User Story:** As a contributor, I want explicit feature-contract checks for
no-std / alloc-only crates so that feature-gating regressions are caught before
PR review or CI.

**Requirements:**

- R1: Identify crates whose manifests promise meaningful `--no-default-features`
or alloc-only support
- R2: Add a local and CI-visible verification path that exercises those feature
contracts directly
- R3: Keep the lane scoped so it stays fast enough to run during review-fix
loops
- R4: Document which crates are covered and what the lane is proving

**Acceptance Criteria:**

- [ ] AC1: At least the current runtime-schema / ABI crates have an explicit
`--no-default-features` check
- [ ] AC2: A deliberate `std` leak in a gated crate fails the lane
- [ ] AC3: Contributor docs explain when to run the lane and what it covers
- [ ] AC4: The covered crate list is easy to keep aligned with manifest truth

**Definition of Done:**

- [ ] Code reviewed and merged
- [ ] Tests pass (CI green)
- [ ] Documentation updated (if applicable)

**Scope:** Feature-gate verification for crates that claim no-std or alloc-only
support.
**Out of Scope:** Broad workspace-wide no-std support or changing crate feature
semantics.

**Test Plan:**

- **Goldens:** n/a
- **Failures:** Intentionally introduce a `std` dependency in a gated path and
verify the lane fails
- **Edges:** `default-features = false`, alloc-only mode, transitive feature
forwarding
- **Fuzz/Stress:** n/a

**Blocked By:** none
**Blocking:** none

**Est. Hours:** 2h
**Expected Complexity:** ~60 LoC (lane wiring + docs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Define the maintenance entrypoint in the task spec.

Both backlog items describe new repo-facing commands, but neither says they should land under cargo xtask. Leaving that open makes it too easy for the eventual implementation to become another one-off script surface. Add the expected cargo xtask ... shape to the requirements or AC1 for both tasks now.

Based on learnings: "Preferred repo maintenance entrypoint is cargo xtask ... (see xtask/ and .cargo/config.toml)".

Also applies to: 449-493

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/ROADMAP/backlog/tooling-misc.md` around lines 397 - 445, Update the
"T-10-8-10: Feature-Gate Contract Verification" task spec to mandate the
maintenance entrypoint be a cargo xtask command (e.g., "cargo xtask
feature-gate-verify") by adding that shape to the Requirements (R2) and
Acceptance Criteria (AC1) so the verification is implemented under the repo's
xtask surface; apply the same change to the other backlog item in this document
so both tasks explicitly require a cargo xtask entrypoint and mention xtask/ and
.cargo/config.toml as the expected location/configuration.

import { escapeDotString } from "./dag-utils.js";

const INPUT_FILE = "TASKS-DAG.md";
const INPUT_FILE = path.join("docs", "archive", "tasks", "TASKS-DAG.md");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

In Node.js, does path.join("docs","archive","tasks","TASKS-DAG.md") use platform-specific separators on Windows, and does Graphviz DOT treat backslashes inside label strings as escapes?

💡 Result:

Yes, on Windows, path.join("docs","archive","tasks","TASKS-DAG.md") uses platform-specific backslash separators, producing "docs\archive\tasks\TASKS-DAG.md". Yes, Graphviz DOT treats backslashes in label strings as escape characters; a literal backslash must be written as \ in the DOT source.

Citations:


🏁 Script executed:

#!/bin/bash

# First, check the file exists and examine line 10 and surrounding context
echo "=== Line 10 and context ==="
sed -n '1,20p' scripts/generate-tasks-dag.js

echo -e "\n=== Lines 175-185 context ==="
sed -n '170,190p' scripts/generate-tasks-dag.js

Repository: flyingrobots/echo

Length of output: 1671


🏁 Script executed:

#!/bin/bash

# Find and display escapeDotString function implementation
echo "=== Searching for escapeDotString function ==="
rg -n "escapeDotString" scripts/generate-tasks-dag.js -B 2 -A 8

Repository: flyingrobots/echo

Length of output: 2800


🏁 Script executed:

#!/bin/bash

# Check if generated output files are in version control
echo "=== Checking for generated DOT/graph files in repo ==="
git ls-files | grep -E '\.(dot|svg|png|gv)$'

echo -e "\n=== Checking git status for potential generated files ==="
git status --short | head -20

Repository: flyingrobots/echo

Length of output: 4717


🏁 Script executed:

#!/bin/bash

# Find and display escapeDotString function implementation
echo "=== escapeDotString function in dag-utils.js ==="
find scripts -name "dag-utils.js" -type f | head -1 | xargs cat

Repository: flyingrobots/echo

Length of output: 1614


Separate filesystem path from DOT display path to ensure deterministic output.

INPUT_FILE serves double duty: filesystem access and DOT label emission. On Windows, path.join() produces backslashes, which escapeDotString() then doubles for DOT escaping, resulting in platform-dependent committed output. Keep filesystem paths OS-native and display paths POSIX-normalized for reproducible generated artifacts.

Suggested fix
-const INPUT_FILE = path.join("docs", "archive", "tasks", "TASKS-DAG.md");
+const INPUT_FILE_DISPLAY = "docs/archive/tasks/TASKS-DAG.md";
+const INPUT_FILE = path.join(...INPUT_FILE_DISPLAY.split("/"));
@@
-    `  label="Echo — Tasks DAG (from ${escapeDotString(INPUT_FILE)})\\nGenerated by scripts/generate-tasks-dag.js";`,
+    `  label="Echo — Tasks DAG (from ${escapeDotString(INPUT_FILE_DISPLAY)})\\nGenerated by scripts/generate-tasks-dag.js";`,
@@
-    `    LG [label="confirmed in ${escapeDotString(INPUT_FILE)}", color="green", fontcolor="green"];`,
+    `    LG [label="confirmed in ${escapeDotString(INPUT_FILE_DISPLAY)}", color="green", fontcolor="green"];`,

Also applies to: 175-185

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/generate-tasks-dag.js` at line 10, INPUT_FILE is currently used both
for reading the filesystem and for emitting DOT labels, which causes Windows
backslashes to be escaped and produce non-deterministic output; fix by
separating the two concerns: keep a filesystem-native path (e.g., INPUT_FILE_FS
= path.join(...)) for fs operations and create a POSIX-normalized display path
(e.g., INPUT_FILE_DISPLAY = path.posix.join(...) or use path.posix.normalize)
for any DOT label emission passed into escapeDotString(), and update all places
that emit file paths to use INPUT_FILE_DISPLAY (including the similar uses
around the escapeDotString() calls referenced in the 175-185 area).

Comment on lines +11 to +17
const DEFINITION_KIND_NAMES = new Map([
["ScalarTypeDefinition", "scalar"],
["ObjectTypeDefinition", "type"],
["InputObjectTypeDefinition", "input"],
["EnumTypeDefinition", "enum"],
["UnionTypeDefinition", "union"],
]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

InterfaceTypeDefinition is not handled.

The DEFINITION_KIND_NAMES map omits InterfaceTypeDefinition. If the runtime schema ever includes GraphQL interfaces, they won't be collected as definitions, causing false "missing referenced type" errors when other types implement them.

Proposed fix
 const DEFINITION_KIND_NAMES = new Map([
     ["ScalarTypeDefinition", "scalar"],
     ["ObjectTypeDefinition", "type"],
     ["InputObjectTypeDefinition", "input"],
     ["EnumTypeDefinition", "enum"],
     ["UnionTypeDefinition", "union"],
+    ["InterfaceTypeDefinition", "interface"],
 ]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const DEFINITION_KIND_NAMES = new Map([
["ScalarTypeDefinition", "scalar"],
["ObjectTypeDefinition", "type"],
["InputObjectTypeDefinition", "input"],
["EnumTypeDefinition", "enum"],
["UnionTypeDefinition", "union"],
]);
const DEFINITION_KIND_NAMES = new Map([
["ScalarTypeDefinition", "scalar"],
["ObjectTypeDefinition", "type"],
["InputObjectTypeDefinition", "input"],
["EnumTypeDefinition", "enum"],
["UnionTypeDefinition", "union"],
["InterfaceTypeDefinition", "interface"],
]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/validate-runtime-schema-fragments.mjs` around lines 11 - 17,
DEFINITION_KIND_NAMES currently omits GraphQL interfaces so nodes with kind
"InterfaceTypeDefinition" won't be recognized; update the DEFINITION_KIND_NAMES
Map (the constant named DEFINITION_KIND_NAMES in
scripts/validate-runtime-schema-fragments.mjs) to include
["InterfaceTypeDefinition", "interface"] so interface definitions are collected
like other types and won't trigger false missing-type errors.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (6)
schemas/runtime/artifact-a-identifiers.graphql (1)

17-23: ⚠️ Potential issue | 🟠 Major

WorldlineId scalar is still underspecified for frozen ABI/schema parity.

Line 20 keeps opacity semantics, but the fixed-width invariant is missing. For Phase 8 freeze, document WorldlineId as exactly 32 bytes (opaque, no ordering semantics), otherwise generators and adapters can drift on accepted payload width.

Proposed doc fix
 """
 Opaque stable identifier for a worldline.
 
-Semantic rule: this remains an opaque identifier even though implementations
-often derive it from the initial worldline state.
+Semantic rule: this is a fixed-width 32-byte opaque identifier. Consumers must
+preserve opacity and accept/emit exactly 32 bytes (256 bits), even though
+implementations may derive it from initial worldline state.
 """
 scalar WorldlineId
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@schemas/runtime/artifact-a-identifiers.graphql` around lines 17 - 23, The
WorldlineId scalar is underspecified: update the scalar documentation for scalar
WorldlineId to state it is an opaque, fixed-width 32-byte identifier (exactly 32
bytes), with no ordering semantics, to guarantee frozen ABI/schema parity for
Phase 8; modify the comment block above scalar WorldlineId to explicitly
document "exactly 32 bytes (opaque, no ordering semantics)" and include any
expected external encoding note (e.g., raw 32 bytes or canonical hex/base64) so
generators and adapters have a clear payload width.
crates/echo-wasm-abi/src/lib.rs (1)

440-482: ⚠️ Potential issue | 🟡 Minor

Freeze HeadId wire shape explicitly.

The new SetHeadEligibility round-trip only proves semantic equality after decode. It would still pass if HeadId / WriterHeadKey drifted from CBOR bytes(32) to another serde shape, so this file only pins the frozen wire shape for WorldlineId right now. Add the same bytes(32) and 31/33-byte rejection coverage for the HeadId path.

Also applies to: 484-545

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/echo-wasm-abi/src/lib.rs` around lines 440 - 482, The
test_control_intent_round_trip needs to explicitly pin HeadId's CBOR wire shape
like WorldlineId does: update the SetHeadEligibility branch to (1) assert that
packing/unpacking preserves a HeadId constructed from exactly 32 bytes (e.g.
HeadId::from_bytes([2u8;32])) and (2) add negative-case assertions that
decoding/reconstruction rejects 31-byte and 33-byte inputs for HeadId (mirror
the WorldlineId byte-length checks), using the same helpers
pack_control_intent_v1/unpack_control_intent_v1 and the WriterHeadKey type so
the test fails if HeadId’s serde wire shape drifts.
docs/ROADMAP/backlog/tooling-misc.md (1)

405-419: ⚠️ Potential issue | 🟡 Minor

Pin both new tooling tasks to cargo xtask.

Both specs still say “one command” / “local and CI-visible path” without naming the repo maintenance entrypoint. Leave that vague and the eventual implementation can easily become another standalone script surface. Require explicit cargo xtask ... commands in the Requirements and AC for both tasks. Based on learnings: "Preferred repo maintenance entrypoint is cargo xtask ... (see xtask/ and .cargo/config.toml)."

Also applies to: 457-469

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/ROADMAP/backlog/tooling-misc.md` around lines 405 - 419, Update the
roadmap entries (the "R1-R4" checklist and the corresponding Acceptance
Criteria) to require invoking both new tooling tasks via the repository xtask
entrypoint (i.e., prepend "cargo xtask ..." to the
one-command/local-and-CI-visible paths), and update AC items (AC1–AC4 and the
similar block later in the file) to explicitly state "run via cargo xtask" and
reference the repo xtask surface (xtask/ and .cargo/config.toml) so implementers
know where the command lives; ensure both task specs and acceptance checks are
changed consistently wherever the tasks are described in the document.
scripts/generate-tasks-dag.js (1)

10-10: ⚠️ Potential issue | 🟡 Minor

Separate the filesystem path from the DOT display path.

INPUT_FILE now feeds both file I/O and label text. On Windows, path.join() will emit backslashes, so the generated DOT/SVG labels become platform-dependent once passed through escapeDotString(). Keep a native path for reads and a POSIX-normalized path for emitted labels.

Suggested fix
-const INPUT_FILE = path.join("docs", "archive", "tasks", "TASKS-DAG.md");
+const INPUT_FILE_DISPLAY = "docs/archive/tasks/TASKS-DAG.md";
+const INPUT_FILE = path.join(...INPUT_FILE_DISPLAY.split("/"));
@@
-    `  label="Echo — Tasks DAG (from ${escapeDotString(INPUT_FILE)})\\nGenerated by scripts/generate-tasks-dag.js";`,
+    `  label="Echo — Tasks DAG (from ${escapeDotString(INPUT_FILE_DISPLAY)})\\nGenerated by scripts/generate-tasks-dag.js";`,
@@
-    `    LG [label="confirmed in ${escapeDotString(INPUT_FILE)}", color="green", fontcolor="green"];`,
+    `    LG [label="confirmed in ${escapeDotString(INPUT_FILE_DISPLAY)}", color="green", fontcolor="green"];`,
In Node.js, does path.join("docs","archive","tasks","TASKS-DAG.md") use platform-specific separators on Windows, and does Graphviz DOT treat backslashes in label strings as escapes?

Also applies to: 175-185

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/generate-tasks-dag.js` at line 10, INPUT_FILE is currently used both
for filesystem reads and as the label text, causing backslashes on Windows to
leak into DOT labels; change the code to keep a native path for I/O (leave
INPUT_FILE = path.join(...)) and create a separate POSIX-normalized label path
(e.g., use path.posix.join or replace backslashes via
path.resolve(...).split(path.sep).join("/")) for any text passed to
escapeDotString()/DOT generation (update all usages where INPUT_FILE is passed
into escapeDotString or label creation, including the later block around the
previous 175-185 region) so labels always use forward slashes while file I/O
still uses the platform-native path.
scripts/validate-runtime-schema-fragments.mjs (1)

11-17: ⚠️ Potential issue | 🟡 Minor

InterfaceTypeDefinition is still missing.

If the runtime schema ever includes GraphQL interfaces, they won't be collected, causing false "missing referenced type" errors.

Fix
 const DEFINITION_KIND_NAMES = new Map([
     ["ScalarTypeDefinition", "scalar"],
     ["ObjectTypeDefinition", "type"],
     ["InputObjectTypeDefinition", "input"],
     ["EnumTypeDefinition", "enum"],
     ["UnionTypeDefinition", "union"],
+    ["InterfaceTypeDefinition", "interface"],
 ]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/validate-runtime-schema-fragments.mjs` around lines 11 - 17,
DEFINITION_KIND_NAMES map is missing "InterfaceTypeDefinition" which causes
GraphQL interfaces to be uncollected and trigger false "missing referenced type"
errors; add an entry mapping "InterfaceTypeDefinition" to "interface" in the
DEFINITION_KIND_NAMES Map so interface definitions are recognized (update the
constant DEFINITION_KIND_NAMES where the other kind-name pairs are defined).
crates/echo-runtime-schema/src/lib.rs (1)

164-170: ⚠️ Potential issue | 🟡 Minor

Add local regression coverage for the non-canonical runtime-id decodes.

This module owns the exact-length serde contract, but the tests only pin happy-path CBOR byte strings. Add local cases for 31-byte and 33-byte byte strings plus 32-element and 33-element u8 sequences so decode_runtime_id() and visit_seq() cannot drift behind downstream ABI coverage.

Also applies to: 214-228, 340-385

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/echo-runtime-schema/src/lib.rs` around lines 164 - 170, Add unit tests
that assert decoding rejects non-canonical lengths: add cases that attempt to
deserialize 31-byte and 33-byte byte strings and 32-element and 33-element u8
sequences to ensure decode_runtime_id() and visit_seq() enforce exact 32-byte
length; specifically, create tests that call the deserializer on CBOR/serde
inputs representing a 31-byte bytes, a 33-byte bytes, a seq of 32 u8s (valid)
and a seq of 33 u8s (invalid) and assert success only for the 32-byte cases and
errors for others, which will exercise decode_runtime_id() and the visitor
implementation (visit_seq) to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/echo-runtime-schema/src/lib.rs`:
- Line 35: The struct currently exposes its inner counter as pub (pub struct
$name(pub u64)), allowing callers to bypass checked arithmetic; change the inner
field to private (e.g., pub struct $name(u64)) and ensure all external access
uses the existing from_raw and as_u64 functions (or associated methods) as the
documented escape hatches; update any constructors or direct field accesses in
code to call $name::from_raw(...) and use .as_u64() instead of referencing .0 so
all arithmetic goes through the checked-arithmetic API boundary.

In `@docs/guide/cargo-features.md`:
- Around line 80-90: The documentation snapshot metadata at the top still reads
"generated as of 2026-03-07" and is now stale; update that generated-snapshot
date string to the current correct date to match the new `echo-runtime-schema`
section content (search for the literal "generated as of 2026-03-07" in
docs/guide/cargo-features.md and replace it with the updated date), ensuring the
snapshot date reflects when the file was regenerated.

In `@docs/plans/phase-8-schema-freeze-inventory.md`:
- Around line 97-106: The Artifact A documentation list is missing the
scalar/type PositiveInt; update the Artifact A inventory to include
`PositiveInt` alongside HeadId, WorldlineId, WriterHeadKey, IntentKind,
WorldlineTick, and GlobalTick so that the listing matches the definition in
artifact-a-identifiers.graphql (where PositiveInt is declared) and reflects its
use (PositiveInt!) in artifact-b and artifact-d.

In `@schemas/runtime/artifact-d-scheduler-results.graphql`:
- Around line 91-100: The SchedulerStatus type lacks in-schema documentation of
nullability semantics for fields clients branch on; update the SchedulerStatus
definition (fields: activeMode, runId, latestCycleGlobalTick,
latestCommitGlobalTick, lastQuiescentGlobalTick, lastRunCompletion) to include
short GraphQL field descriptions that state what a null vs non-null value means
(e.g., null = scheduler inactive, null = pre-first-run, null = impossible given
current state), and if any field should be non-null by invariant, make its type
non-nullable (or explicitly document why it can be null) so the SDL itself
encodes the state-coupled behavior alongside the field (use GraphQL description
strings or comments directly above each field in the SchedulerStatus type).

---

Duplicate comments:
In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 164-170: Add unit tests that assert decoding rejects non-canonical
lengths: add cases that attempt to deserialize 31-byte and 33-byte byte strings
and 32-element and 33-element u8 sequences to ensure decode_runtime_id() and
visit_seq() enforce exact 32-byte length; specifically, create tests that call
the deserializer on CBOR/serde inputs representing a 31-byte bytes, a 33-byte
bytes, a seq of 32 u8s (valid) and a seq of 33 u8s (invalid) and assert success
only for the 32-byte cases and errors for others, which will exercise
decode_runtime_id() and the visitor implementation (visit_seq) to prevent
regressions.

In `@crates/echo-wasm-abi/src/lib.rs`:
- Around line 440-482: The test_control_intent_round_trip needs to explicitly
pin HeadId's CBOR wire shape like WorldlineId does: update the
SetHeadEligibility branch to (1) assert that packing/unpacking preserves a
HeadId constructed from exactly 32 bytes (e.g. HeadId::from_bytes([2u8;32])) and
(2) add negative-case assertions that decoding/reconstruction rejects 31-byte
and 33-byte inputs for HeadId (mirror the WorldlineId byte-length checks), using
the same helpers pack_control_intent_v1/unpack_control_intent_v1 and the
WriterHeadKey type so the test fails if HeadId’s serde wire shape drifts.

In `@docs/ROADMAP/backlog/tooling-misc.md`:
- Around line 405-419: Update the roadmap entries (the "R1-R4" checklist and the
corresponding Acceptance Criteria) to require invoking both new tooling tasks
via the repository xtask entrypoint (i.e., prepend "cargo xtask ..." to the
one-command/local-and-CI-visible paths), and update AC items (AC1–AC4 and the
similar block later in the file) to explicitly state "run via cargo xtask" and
reference the repo xtask surface (xtask/ and .cargo/config.toml) so implementers
know where the command lives; ensure both task specs and acceptance checks are
changed consistently wherever the tasks are described in the document.

In `@schemas/runtime/artifact-a-identifiers.graphql`:
- Around line 17-23: The WorldlineId scalar is underspecified: update the scalar
documentation for scalar WorldlineId to state it is an opaque, fixed-width
32-byte identifier (exactly 32 bytes), with no ordering semantics, to guarantee
frozen ABI/schema parity for Phase 8; modify the comment block above scalar
WorldlineId to explicitly document "exactly 32 bytes (opaque, no ordering
semantics)" and include any expected external encoding note (e.g., raw 32 bytes
or canonical hex/base64) so generators and adapters have a clear payload width.

In `@scripts/generate-tasks-dag.js`:
- Line 10: INPUT_FILE is currently used both for filesystem reads and as the
label text, causing backslashes on Windows to leak into DOT labels; change the
code to keep a native path for I/O (leave INPUT_FILE = path.join(...)) and
create a separate POSIX-normalized label path (e.g., use path.posix.join or
replace backslashes via path.resolve(...).split(path.sep).join("/")) for any
text passed to escapeDotString()/DOT generation (update all usages where
INPUT_FILE is passed into escapeDotString or label creation, including the later
block around the previous 175-185 region) so labels always use forward slashes
while file I/O still uses the platform-native path.

In `@scripts/validate-runtime-schema-fragments.mjs`:
- Around line 11-17: DEFINITION_KIND_NAMES map is missing
"InterfaceTypeDefinition" which causes GraphQL interfaces to be uncollected and
trigger false "missing referenced type" errors; add an entry mapping
"InterfaceTypeDefinition" to "interface" in the DEFINITION_KIND_NAMES Map so
interface definitions are recognized (update the constant DEFINITION_KIND_NAMES
where the other kind-name pairs are defined).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5c9510ed-4f96-4019-92af-60ebd00a54e5

📥 Commits

Reviewing files that changed from the base of the PR and between 12b4cab and d560ba2.

⛔ Files ignored due to path filters (4)
  • Cargo.lock is excluded by !**/*.lock
  • docs/assets/dags/tasks-dag.dot is excluded by !**/*.dot
  • docs/assets/dags/tasks-dag.svg is excluded by !**/*.svg
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (58)
  • .gitignore
  • .vscode/settings.json
  • CHANGELOG.md
  • CONTRIBUTING.md
  • Cargo.toml
  • crates/echo-dind-harness/tests/digest_golden_vectors.rs
  • crates/echo-runtime-schema/Cargo.toml
  • crates/echo-runtime-schema/README.md
  • crates/echo-runtime-schema/src/lib.rs
  • crates/echo-wasm-abi/Cargo.toml
  • crates/echo-wasm-abi/src/codec.rs
  • crates/echo-wasm-abi/src/kernel_port.rs
  • crates/echo-wasm-abi/src/lib.rs
  • crates/ttd-browser/src/lib.rs
  • crates/warp-core/Cargo.toml
  • crates/warp-core/src/clock.rs
  • crates/warp-core/src/coordinator.rs
  • crates/warp-core/src/engine_impl.rs
  • crates/warp-core/src/head.rs
  • crates/warp-core/src/head_inbox.rs
  • crates/warp-core/src/observation.rs
  • crates/warp-core/src/playback.rs
  • crates/warp-core/src/provenance_store.rs
  • crates/warp-core/src/snapshot.rs
  • crates/warp-core/src/worldline.rs
  • crates/warp-core/src/worldline_registry.rs
  • crates/warp-core/src/worldline_state.rs
  • crates/warp-core/tests/checkpoint_fork_tests.rs
  • crates/warp-core/tests/common/mod.rs
  • crates/warp-core/tests/golden_vectors_phase0.rs
  • crates/warp-core/tests/inbox.rs
  • crates/warp-core/tests/invariant_property_tests.rs
  • crates/warp-core/tests/outputs_playback_tests.rs
  • crates/warp-core/tests/slice_theorem_proof.rs
  • crates/warp-wasm/src/lib.rs
  • crates/warp-wasm/src/warp_kernel.rs
  • docs/ROADMAP/backlog/README.md
  • docs/ROADMAP/backlog/tooling-misc.md
  • docs/dependency-dags.md
  • docs/guide/cargo-features.md
  • docs/plans/adr-0008-and-0009.md
  • docs/plans/phase-8-runtime-schema-conformance.md
  • docs/plans/phase-8-runtime-schema-mapping-contract.md
  • docs/plans/phase-8-schema-freeze-inventory.md
  • docs/spec/SPEC-0009-wasm-abi-v3.md
  • package.json
  • schemas/runtime/README.md
  • schemas/runtime/artifact-a-identifiers.graphql
  • schemas/runtime/artifact-b-routing-and-admission.graphql
  • schemas/runtime/artifact-c-playback-control.graphql
  • schemas/runtime/artifact-d-scheduler-results.graphql
  • scripts/check-append-only.js
  • scripts/generate-dependency-dags.js
  • scripts/generate-tasks-dag.js
  • scripts/validate-runtime-schema-fragments.mjs
  • scripts/verify-local.sh
  • tests/hooks/test_dependency_dags.sh
  • tests/hooks/test_runtime_schema_validation.sh

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Default)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[cfg_attr(feature = "serde", serde(transparent))]
pub struct $name(pub u64);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Examine the file around line 35
head -80 crates/echo-runtime-schema/src/lib.rs | tail -50

Repository: flyingrobots/echo

Length of output: 1727


🏁 Script executed:

# Get the full macro definition to understand what's being generated
sed -n '1,100p' crates/echo-runtime-schema/src/lib.rs

Repository: flyingrobots/echo

Length of output: 3246


🏁 Script executed:

# Search for from_raw and as_u64 references in the file
rg "from_raw|as_u64" crates/echo-runtime-schema/src/lib.rs -A 2 -B 2

Repository: flyingrobots/echo

Length of output: 1020


🏁 Script executed:

# Find where the logical_counter! macro is invoked to see if the generated types have rustdoc
rg "logical_counter!" crates/echo-runtime-schema/src/lib.rs -A 1 -B 1

Repository: flyingrobots/echo

Length of output: 309


🏁 Script executed:

# Check if there's rustdoc on the macro definition itself
sed -n '20,32p' crates/echo-runtime-schema/src/lib.rs

Repository: flyingrobots/echo

Length of output: 416


Make the logical counter field private to enforce the checked-arithmetic API boundary.

pub struct $name(pub u64); allows callers to bypass checked arithmetic via direct construction Type(value) and field access .0. Change the field to private and route all access through from_raw and as_u64, which already exist as documented escape hatches.

Suggested change
-        pub struct $name(pub u64);
+        pub struct $name(u64);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pub struct $name(pub u64);
pub struct $name(u64);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/echo-runtime-schema/src/lib.rs` at line 35, The struct currently
exposes its inner counter as pub (pub struct $name(pub u64)), allowing callers
to bypass checked arithmetic; change the inner field to private (e.g., pub
struct $name(u64)) and ensure all external access uses the existing from_raw and
as_u64 functions (or associated methods) as the documented escape hatches;
update any constructors or direct field accesses in code to call
$name::from_raw(...) and use .as_u64() instead of referencing .0 so all
arithmetic goes through the checked-arithmetic API boundary.

Comment on lines +80 to +90
| Feature | Default | Description |
| ------- | ------- | ---------------------------------------------------------------------------------------------------------- |
| `std` | **yes** | Standard library support (enables `serde/std`, `ciborium/std`, `half/std`, and `echo-runtime-schema/std`). |
| `alloc` | no | Alloc-only mode for `no_std` environments. |

## echo-runtime-schema

| Feature | Default | Description |
| ------- | ------- | --------------------------------------------------------------------------------------------------------------------------------------------------- |
| `std` | **yes** | Standard library support. |
| `serde` | **yes** | Enables serde derives for shared runtime-schema ids, counters, and structural keys. Consumers opt in explicitly when default features are disabled. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Update generated snapshot date to match the new feature content.

The new echo-runtime-schema section is current, but the file still claims it is generated “as of 2026-03-07” (Line 6), which is now stale metadata.

Proposed patch
-Generated from `Cargo.toml` files as of 2026-03-07. Run
+Generated from `Cargo.toml` files as of 2026-03-24. Run

As per coding guidelines, docs/**/*.md: Documentation accuracy matters — especially stale cross-references and factual accuracy.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/guide/cargo-features.md` around lines 80 - 90, The documentation
snapshot metadata at the top still reads "generated as of 2026-03-07" and is now
stale; update that generated-snapshot date string to the current correct date to
match the new `echo-runtime-schema` section content (search for the literal
"generated as of 2026-03-07" in docs/guide/cargo-features.md and replace it with
the updated date), ensuring the snapshot date reflects when the file was
regenerated.

Comment on lines +97 to +106
### Artifact A: Runtime identifiers and logical counters

- `HeadId`
- `WorldlineId`
- `WriterHeadKey`
- `IntentKind`
- `WorldlineTick`
- `GlobalTick`

Source file: `schemas/runtime/artifact-a-identifiers.graphql`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if PositiveInt is defined in artifact-a-identifiers.graphql
echo "=== Checking PositiveInt definition in artifact-a ==="
grep -n "PositiveInt" schemas/runtime/artifact-a-identifiers.graphql || echo "Not found in artifact-a"

echo ""
echo "=== Checking all PositiveInt references ==="
grep -rn "PositiveInt" schemas/runtime/

Repository: flyingrobots/echo

Length of output: 597


Add PositiveInt to the Artifact A listing.

The documentation at lines 97-106 omits PositiveInt, which is defined at line 56 of artifact-a-identifiers.graphql. Since PositiveInt! is used in artifact-b and artifact-d, it must be included in the Artifact A inventory for completeness.

Updated snippet:
### Artifact A: Runtime identifiers and logical counters

- `HeadId`
- `WorldlineId`
- `WriterHeadKey`
- `IntentKind`
- `WorldlineTick`
- `GlobalTick`
- `PositiveInt`

Source file: `schemas/runtime/artifact-a-identifiers.graphql`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/plans/phase-8-schema-freeze-inventory.md` around lines 97 - 106, The
Artifact A documentation list is missing the scalar/type PositiveInt; update the
Artifact A inventory to include `PositiveInt` alongside HeadId, WorldlineId,
WriterHeadKey, IntentKind, WorldlineTick, and GlobalTick so that the listing
matches the definition in artifact-a-identifiers.graphql (where PositiveInt is
declared) and reflects its use (PositiveInt!) in artifact-b and artifact-d.

Comment on lines +91 to +100
type SchedulerStatus {
state: SchedulerState!
activeMode: SchedulerMode
workState: WorkState!
runId: RunId
latestCycleGlobalTick: GlobalTick
latestCommitGlobalTick: GlobalTick
lastQuiescentGlobalTick: GlobalTick
lastRunCompletion: RunCompletion
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Document the SchedulerStatus nullability invariants.

activeMode, runId, the tick fields, and lastRunCompletion are the fields clients will branch on, but this fragment does not say whether null means inactive, pre-first-run, or impossible in the current state. Freeze that state-coupled behavior next to the fields, not only in external docs.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@schemas/runtime/artifact-d-scheduler-results.graphql` around lines 91 - 100,
The SchedulerStatus type lacks in-schema documentation of nullability semantics
for fields clients branch on; update the SchedulerStatus definition (fields:
activeMode, runId, latestCycleGlobalTick, latestCommitGlobalTick,
lastQuiescentGlobalTick, lastRunCompletion) to include short GraphQL field
descriptions that state what a null vs non-null value means (e.g., null =
scheduler inactive, null = pre-first-run, null = impossible given current
state), and if any field should be non-null by invariant, make its type
non-nullable (or explicitly document why it can be null) so the SDL itself
encodes the state-coupled behavior alongside the field (use GraphQL description
strings or comments directly above each field in the SchedulerStatus type).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant